[PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

BIOS/platform may use some of the pins by themselves, such as providing SCI
(System Control Interrupt) from the embedded controller. The driver masks
all interrupts at probe time which prevents those pins from triggering
interrupts properly.

Fix this by not masking all interrupts at probe -- it should be enough just
to clear the status register.

Reported-by: Yu C Chen <yu.c.chen@intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 82f691eeeec4..c9c0257a370c 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1417,8 +1417,7 @@ 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);
+	/* Clear all interrupts */
 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
 
 	ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, 0,
-- 
2.1.4

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Fri, May 22, 2015 at 10:56:08AM +0300, Mika Westerberg wrote:
> BIOS/platform may use some of the pins by themselves, such as providing SCI
> (System Control Interrupt) from the embedded controller. The driver masks
> all interrupts at probe time which prevents those pins from triggering
> interrupts properly.
> 
> Fix this by not masking all interrupts at probe -- it should be enough just
> to clear the status register.
> 
> Reported-by: Yu C Chen <yu.c.chen@intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Please ignore this patch for now. It turned out to be causing spurious
interrupts on another platform.

I'll need to rethink how to fix the reported issue.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Linus Walleij]

On Mon, Jun 1, 2015 at 11:23 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, May 22, 2015 at 10:56:08AM +0300, Mika Westerberg wrote:
>> BIOS/platform may use some of the pins by themselves, such as providing SCI
>> (System Control Interrupt) from the embedded controller. The driver masks
>> all interrupts at probe time which prevents those pins from triggering
>> interrupts properly.
>>
>> Fix this by not masking all interrupts at probe -- it should be enough just
>> to clear the status register.
>>
>> Reported-by: Yu C Chen <yu.c.chen@intel.com>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> Please ignore this patch for now. It turned out to be causing spurious
> interrupts on another platform.
>
> I'll need to rethink how to fix the reported issue.

Looks like a case of "embed more magic knowledge" in the driver :/

It needs to know what platform it is running on, and only leave specific
bits unmasked on these specific platforms. Right? Thereby
tossing all of the acpi_device_id matching and abstraction out
of the window.

(Yes, I hate BIOSes doing crap behind my back.)

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Tue, Jun 02, 2015 at 03:53:40PM +0200, Linus Walleij wrote:
> On Mon, Jun 1, 2015 at 11:23 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Fri, May 22, 2015 at 10:56:08AM +0300, Mika Westerberg wrote:
> >> BIOS/platform may use some of the pins by themselves, such as providing SCI
> >> (System Control Interrupt) from the embedded controller. The driver masks
> >> all interrupts at probe time which prevents those pins from triggering
> >> interrupts properly.
> >>
> >> Fix this by not masking all interrupts at probe -- it should be enough just
> >> to clear the status register.
> >>
> >> Reported-by: Yu C Chen <yu.c.chen@intel.com>
> >> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >
> > Please ignore this patch for now. It turned out to be causing spurious
> > interrupts on another platform.
> >
> > I'll need to rethink how to fix the reported issue.
> 
> Looks like a case of "embed more magic knowledge" in the driver :/
> 
> It needs to know what platform it is running on, and only leave specific
> bits unmasked on these specific platforms. Right? Thereby
> tossing all of the acpi_device_id matching and abstraction out
> of the window.

That's right.

We still have few options left, like using ACPI _AEI (ACPI GPIO
triggered events) for this or adding GPIO interrupt support directly to
the EC driver.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Anisse Astier]

Hi Mika,

On Tue, Jun 2, 2015 at 4:15 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Jun 02, 2015 at 03:53:40PM +0200, Linus Walleij wrote:
>> On Mon, Jun 1, 2015 at 11:23 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Fri, May 22, 2015 at 10:56:08AM +0300, Mika Westerberg wrote:
>> >> BIOS/platform may use some of the pins by themselves, such as providing SCI
>> >> (System Control Interrupt) from the embedded controller. The driver masks
>> >> all interrupts at probe time which prevents those pins from triggering
>> >> interrupts properly.
>> >>
>> >> Fix this by not masking all interrupts at probe -- it should be enough just
>> >> to clear the status register.
>> >>
>> >> Reported-by: Yu C Chen <yu.c.chen@intel.com>
>> >> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> >
>> > Please ignore this patch for now. It turned out to be causing spurious
>> > interrupts on another platform.
>> >
>> > I'll need to rethink how to fix the reported issue.
>>
>> Looks like a case of "embed more magic knowledge" in the driver :/
>>
>> It needs to know what platform it is running on, and only leave specific
>> bits unmasked on these specific platforms. Right? Thereby
>> tossing all of the acpi_device_id matching and abstraction out
>> of the window.
>
> That's right.
>
> We still have few options left, like using ACPI _AEI (ACPI GPIO
> triggered events) for this or adding GPIO interrupt support directly to
> the EC driver.

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

Regards,

Anisse

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Tue, Aug 16, 2016 at 06:12:40PM +0200, Anisse Astier wrote:
> Hi Mika,
> 
> On Tue, Jun 2, 2015 at 4:15 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Jun 02, 2015 at 03:53:40PM +0200, Linus Walleij wrote:
> >> On Mon, Jun 1, 2015 at 11:23 AM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > On Fri, May 22, 2015 at 10:56:08AM +0300, Mika Westerberg wrote:
> >> >> BIOS/platform may use some of the pins by themselves, such as providing SCI
> >> >> (System Control Interrupt) from the embedded controller. The driver masks
> >> >> all interrupts at probe time which prevents those pins from triggering
> >> >> interrupts properly.
> >> >>
> >> >> Fix this by not masking all interrupts at probe -- it should be enough just
> >> >> to clear the status register.
> >> >>
> >> >> Reported-by: Yu C Chen <yu.c.chen@intel.com>
> >> >> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >> >
> >> > Please ignore this patch for now. It turned out to be causing spurious
> >> > interrupts on another platform.
> >> >
> >> > I'll need to rethink how to fix the reported issue.
> >>
> >> Looks like a case of "embed more magic knowledge" in the driver :/
> >>
> >> It needs to know what platform it is running on, and only leave specific
> >> bits unmasked on these specific platforms. Right? Thereby
> >> tossing all of the acpi_device_id matching and abstraction out
> >> of the window.
> >
> > That's right.
> >
> > We still have few options left, like using ACPI _AEI (ACPI GPIO
> > triggered events) for this or adding GPIO interrupt support directly to
> > the EC driver.
> 
> 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?

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Anisse Astier]

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
pin 26 (CX_PREQ_B) mode 1 ctrl0 0x00a10301 ctrl1 0x05c00000
pin 27 (SEC_GPIO_SUS9) GPIO ctrl0 0x00918102 ctrl1 0x05c00000
pin 30 (TRST_B) mode 1 ctrl0 0x00a10300 ctrl1 0x05c00000
pin 31 (TCK) mode 1 ctrl0 0x00210300 ctrl1 0x05c00000
pin 32 (PROCHOT_B) mode 1 ctrl0 0x00010301 ctrl1 0x05c00060
pin 33 (SVIDO_DATA) mode 0 ctrl0 0x00000300 ctrl1 0x05c00020
pin 34 (TMS) mode 1 ctrl0 0x00a10301 ctrl1 0x05c00000
pin 35 (CX_PRDY_B_2) mode 1 ctrl0 0x00a10301 ctrl1 0x05c00000
pin 36 (TDO_2) mode 1 ctrl0 0x00010300 ctrl1 0x05c00000
pin 37 (CX_PRDY_B) mode 1 ctrl0 0x00a10301 ctrl1 0x05c00000
pin 38 (SVIDO_ALERT_B) mode 1 ctrl0 0x00010301 ctrl1 0x05c00040
pin 39 (TDO) mode 1 ctrl0 0x00010300 ctrl1 0x05c00000
pin 40 (SVIDO_CLK) mode 1 ctrl0 0x00010300 ctrl1 0x05c00000
pin 41 (TDI) mode 1 ctrl0 0x00a10301 ctrl1 0x05c00000
pin 45 (GP_CAMERASB_05) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 46 (GP_CAMERASB_02) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 47 (GP_CAMERASB_08) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 48 (GP_CAMERASB_00) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 49 (GP_CAMERASB_06) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 50 (GP_CAMERASB_10) GPIO ctrl0 0x00118100 ctrl1 0x05c00000
pin 51 (GP_CAMERASB_03) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 52 (GP_CAMERASB_09) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 53 (GP_CAMERASB_01) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 54 (GP_CAMERASB_07) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 55 (GP_CAMERASB_11) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 56 (GP_CAMERASB_04) GPIO ctrl0 0x00118100 ctrl1 0x05c00020
pin 60 (PANEL0_BKLTEN) mode 1 ctrl0 0x00010300 ctrl1 0x05c00000
pin 61 (HV_DDI0_HPD) mode 1 ctrl0 0x03110301 ctrl1 0x05c00020
pin 62 (HV_DDI2_DDC_SDA) mode 3 ctrl0 0x00930301 ctrl1 0x05c00000
pin 63 (PANEL1_BKLTCTL) GPIO ctrl0 0x00018102 ctrl1 0x05c00000
pin 64 (HV_DDI1_HPD) mode 1 ctrl0 0x03010300 ctrl1 0x05c00020
pin 65 (PANEL0_BKLTCTL) mode 1 ctrl0 0x00010300 ctrl1 0x05c00000
pin 66 (HV_DDI0_DDC_SDA) mode 1 ctrl0 0x00910301 ctrl1 0x05c00000
pin 67 (HV_DDI2_DDC_SCL) mode 3 ctrl0 0x00930301 ctrl1 0x05c00000
pin 68 (HV_DDI2_HPD) mode 1 ctrl0 0x03110300 ctrl1 0x05c00020
pin 69 (PANEL1_VDDEN) mode 1 ctrl0 0x00010300 ctrl1 0x05c00000
pin 70 (PANEL1_BKLTEN) mode 1 ctrl0 0x00010300 ctrl1 0x05c00000
pin 71 (HV_DDI0_DDC_SCL) mode 1 ctrl0 0x00910301 ctrl1 0x05c00000
pin 72 (PANEL0_VDDEN) mode 1 ctrl0 0x00010300 ctrl1 0x05c00000

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

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?

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,

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Anisse Astier]

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

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

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

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Phidias Chiang]

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.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

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/

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Phidias Chiang]

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.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

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?

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Phidias Chiang]

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.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

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.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Fri, Sep 09, 2016 at 11:58:32AM +0300, Mika Westerberg wrote:
> 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?

Something like this:

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 0fe8fad..95fa3b1 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1357,6 +1357,11 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
 		value |= BIT(intr_line);
 	chv_writel(value, pctrl->regs + CHV_INTMASK);
 
+	if (printk_ratelimit()) {
+		dev_info(pctrl->dev, "%smask pin %u intmask 0x%08x\n",
+			 mask ? "" : "un", pin, readl(pctrl->regs + CHV_INTMASK));
+	}
+
 	raw_spin_unlock_irqrestore(&chv_lock, flags);
 }
 

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Phidias Chiang]

On Sun, Sep 11, 2016 at 11:05:06AM +0300, Mika Westerberg wrote:
> On Fri, Sep 09, 2016 at 11:58:32AM +0300, Mika Westerberg wrote:
> > On Fri, Sep 09, 2016 at 04:23:58PM +0800, Phidias Chiang wrote:
> > 
> > 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?
> 
> Something like this:
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 0fe8fad..95fa3b1 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1357,6 +1357,11 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
>  		value |= BIT(intr_line);
>  	chv_writel(value, pctrl->regs + CHV_INTMASK);
>  
> +	if (printk_ratelimit()) {
> +		dev_info(pctrl->dev, "%smask pin %u intmask 0x%08x\n",
> +			 mask ? "" : "un", pin, readl(pctrl->regs + CHV_INTMASK));
> +	}
> +
>  	raw_spin_unlock_irqrestore(&chv_lock, flags);
>  }
>  

With printk_ratelimit():

[    2.058485] cherryview-pinctrl INT33FF:00: INTMASK0: 0x00000006
[    2.058513] cherryview-pinctrl INT33FF:00: mask pin 0 intmask 0x00000006
[    2.058533] cherryview-pinctrl INT33FF:00: mask pin 1 intmask 0x00000006
[    2.058551] cherryview-pinctrl INT33FF:00: mask pin 2 intmask 0x00000006
[    2.058569] cherryview-pinctrl INT33FF:00: mask pin 3 intmask 0x00000006
[    2.058587] cherryview-pinctrl INT33FF:00: mask pin 4 intmask 0x00000006
[    2.058604] cherryview-pinctrl INT33FF:00: mask pin 5 intmask 0x00000006
[    2.058623] cherryview-pinctrl INT33FF:00: mask pin 6 intmask 0x00000006
[    2.058641] cherryview-pinctrl INT33FF:00: mask pin 7 intmask 0x00000006
[    2.058663] cherryview-pinctrl INT33FF:00: mask pin 15 intmask 0x00000006
[    2.059401] cherryview-pinctrl INT33FF:00: INTMASK1: 0x00000000
[    2.059551] cherryview-pinctrl INT33FF:01: INTMASK0: 0x00004000
[    2.061272] cherryview-pinctrl INT33FF:01: INTMASK1: 0x00000000
[    2.061516] cherryview-pinctrl INT33FF:02: INTMASK0: 0x00000000
[    2.061906] cherryview-pinctrl INT33FF:02: INTMASK1: 0x00000000
[    2.062116] cherryview-pinctrl INT33FF:03: INTMASK0: 0x00000000
[    2.062956] cherryview-pinctrl INT33FF:03: INTMASK1: 0x00000000

w/o printk_ratelimit():
http://pastebin.com/dLDELDB4

The main difference is the intmask turns to 0x4 on pin 75 and 0x0
afterwards for INT33FF:00. And same thing happened with :01 on pin 25

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Mon, Sep 12, 2016 at 02:56:56PM +0800, Phidias Chiang wrote:
> On Sun, Sep 11, 2016 at 11:05:06AM +0300, Mika Westerberg wrote:
> > On Fri, Sep 09, 2016 at 11:58:32AM +0300, Mika Westerberg wrote:
> > > On Fri, Sep 09, 2016 at 04:23:58PM +0800, Phidias Chiang wrote:
> > > 
> > > 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?
> > 
> > Something like this:
> > 
> > diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> > index 0fe8fad..95fa3b1 100644
> > --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> > +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> > @@ -1357,6 +1357,11 @@ static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
> >  		value |= BIT(intr_line);
> >  	chv_writel(value, pctrl->regs + CHV_INTMASK);
> >  
> > +	if (printk_ratelimit()) {
> > +		dev_info(pctrl->dev, "%smask pin %u intmask 0x%08x\n",
> > +			 mask ? "" : "un", pin, readl(pctrl->regs + CHV_INTMASK));
> > +	}
> > +
> >  	raw_spin_unlock_irqrestore(&chv_lock, flags);
> >  }
> >  
> 
> With printk_ratelimit():
> 
> [    2.058485] cherryview-pinctrl INT33FF:00: INTMASK0: 0x00000006
> [    2.058513] cherryview-pinctrl INT33FF:00: mask pin 0 intmask 0x00000006
> [    2.058533] cherryview-pinctrl INT33FF:00: mask pin 1 intmask 0x00000006
> [    2.058551] cherryview-pinctrl INT33FF:00: mask pin 2 intmask 0x00000006
> [    2.058569] cherryview-pinctrl INT33FF:00: mask pin 3 intmask 0x00000006
> [    2.058587] cherryview-pinctrl INT33FF:00: mask pin 4 intmask 0x00000006
> [    2.058604] cherryview-pinctrl INT33FF:00: mask pin 5 intmask 0x00000006
> [    2.058623] cherryview-pinctrl INT33FF:00: mask pin 6 intmask 0x00000006
> [    2.058641] cherryview-pinctrl INT33FF:00: mask pin 7 intmask 0x00000006
> [    2.058663] cherryview-pinctrl INT33FF:00: mask pin 15 intmask 0x00000006
> [    2.059401] cherryview-pinctrl INT33FF:00: INTMASK1: 0x00000000
> [    2.059551] cherryview-pinctrl INT33FF:01: INTMASK0: 0x00004000
> [    2.061272] cherryview-pinctrl INT33FF:01: INTMASK1: 0x00000000
> [    2.061516] cherryview-pinctrl INT33FF:02: INTMASK0: 0x00000000
> [    2.061906] cherryview-pinctrl INT33FF:02: INTMASK1: 0x00000000
> [    2.062116] cherryview-pinctrl INT33FF:03: INTMASK0: 0x00000000
> [    2.062956] cherryview-pinctrl INT33FF:03: INTMASK1: 0x00000000
> 
> w/o printk_ratelimit():
> http://pastebin.com/dLDELDB4
> 
> The main difference is the intmask turns to 0x4 on pin 75 and 0x0
> afterwards for INT33FF:00. And same thing happened with :01 on pin 25

OK, I see what is going on now. When I changed handle_simple_irq to
handle_bad_irq, the IRQ core in __irq_do_set_handler() thinks the
handler is uninstalled and masks the line.

If you change handle_bad_irq to handle_simple_irq, in call to
gpiochip_irqchip_add(), does it work then?

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Phidias Chiang]

On Mon, Sep 12, 2016 at 12:04:01PM +0300, Mika Westerberg wrote:
> 
> OK, I see what is going on now. When I changed handle_simple_irq to
> handle_bad_irq, the IRQ core in __irq_do_set_handler() thinks the
> handler is uninstalled and masks the line.
> 
> If you change handle_bad_irq to handle_simple_irq, in call to
> gpiochip_irqchip_add(), does it work then?

Yes it does :), thank you for the support!

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Mon, Sep 12, 2016 at 09:04:44PM +0800, Phidias Chiang wrote:
> On Mon, Sep 12, 2016 at 12:04:01PM +0300, Mika Westerberg wrote:
> > 
> > OK, I see what is going on now. When I changed handle_simple_irq to
> > handle_bad_irq, the IRQ core in __irq_do_set_handler() thinks the
> > handler is uninstalled and masks the line.
> > 
> > If you change handle_bad_irq to handle_simple_irq, in call to
> > gpiochip_irqchip_add(), does it work then?
> 
> Yes it does :), thank you for the support!

Thanks for testing.

So we need to use handle_simple_irq here instead.

Linus, do you see any problems with that?

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Linus Walleij]

On Mon, Sep 12, 2016 at 3:11 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Sep 12, 2016 at 09:04:44PM +0800, Phidias Chiang wrote:
>> On Mon, Sep 12, 2016 at 12:04:01PM +0300, Mika Westerberg wrote:
>> >
>> > OK, I see what is going on now. When I changed handle_simple_irq to
>> > handle_bad_irq, the IRQ core in __irq_do_set_handler() thinks the
>> > handler is uninstalled and masks the line.
>> >
>> > If you change handle_bad_irq to handle_simple_irq, in call to
>> > gpiochip_irqchip_add(), does it work then?
>>
>> Yes it does :), thank you for the support!
>
> Thanks for testing.
>
> So we need to use handle_simple_irq here instead.
>
> Linus, do you see any problems with that?

I need to see the patch in its context with a commit message,
I can't figure it out from the thread.

handle_simple_irq() is for something generic not level- or
edge-triggered. If you support specific triggers only, it
should not be used.

Nominally assigning handle_bad_irq() until a specific
edge or level is requested is the right thing to do, since
the IRQ is really not configured for anything at all and
hence has undefined behaviour.

But write a patch and involve the irqchip people I guess?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Tue, Sep 13, 2016 at 11:18:49AM +0200, Linus Walleij wrote:
> On Mon, Sep 12, 2016 at 3:11 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Mon, Sep 12, 2016 at 09:04:44PM +0800, Phidias Chiang wrote:
> >> On Mon, Sep 12, 2016 at 12:04:01PM +0300, Mika Westerberg wrote:
> >> >
> >> > OK, I see what is going on now. When I changed handle_simple_irq to
> >> > handle_bad_irq, the IRQ core in __irq_do_set_handler() thinks the
> >> > handler is uninstalled and masks the line.
> >> >
> >> > If you change handle_bad_irq to handle_simple_irq, in call to
> >> > gpiochip_irqchip_add(), does it work then?
> >>
> >> Yes it does :), thank you for the support!
> >
> > Thanks for testing.
> >
> > So we need to use handle_simple_irq here instead.
> >
> > Linus, do you see any problems with that?
> 
> I need to see the patch in its context with a commit message,
> I can't figure it out from the thread.
> 
> handle_simple_irq() is for something generic not level- or
> edge-triggered. If you support specific triggers only, it
> should not be used.
> 
> Nominally assigning handle_bad_irq() until a specific
> edge or level is requested is the right thing to do, since
> the IRQ is really not configured for anything at all and
> hence has undefined behaviour.

For Cherryview/Braswell some interrupts are actually configured by the
BIOS but they are routed directly to the I/O-APIC and are supposed to be
handled without involvement of the GPIO driver (an example of this is
the ACPI SCI interrupt). However, INTMASK GPIO register can still be
used to mask the interrupt in question.

So when we specify handle_bad_irq as handler the IRQ core thinks the
handler is being uninstalled and masks the interrupt.

> But write a patch and involve the irqchip people I guess?

OK, I'll do this.

Thanks.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Linus Walleij]

On Tue, Sep 13, 2016 at 11:33 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Sep 13, 2016 at 11:18:49AM +0200, Linus Walleij wrote:
>> On Mon, Sep 12, 2016 at 3:11 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Mon, Sep 12, 2016 at 09:04:44PM +0800, Phidias Chiang wrote:
>> >> On Mon, Sep 12, 2016 at 12:04:01PM +0300, Mika Westerberg wrote:
>> >> >
>> >> > OK, I see what is going on now. When I changed handle_simple_irq to
>> >> > handle_bad_irq, the IRQ core in __irq_do_set_handler() thinks the
>> >> > handler is uninstalled and masks the line.
>> >> >
>> >> > If you change handle_bad_irq to handle_simple_irq, in call to
>> >> > gpiochip_irqchip_add(), does it work then?
>> >>
>> >> Yes it does :), thank you for the support!
>> >
>> > Thanks for testing.
>> >
>> > So we need to use handle_simple_irq here instead.
>> >
>> > Linus, do you see any problems with that?
>>
>> I need to see the patch in its context with a commit message,
>> I can't figure it out from the thread.
>>
>> handle_simple_irq() is for something generic not level- or
>> edge-triggered. If you support specific triggers only, it
>> should not be used.
>>
>> Nominally assigning handle_bad_irq() until a specific
>> edge or level is requested is the right thing to do, since
>> the IRQ is really not configured for anything at all and
>> hence has undefined behaviour.
>
> For Cherryview/Braswell some interrupts are actually configured by the
> BIOS but they are routed directly to the I/O-APIC and are supposed to be
> handled without involvement of the GPIO driver (an example of this is
> the ACPI SCI interrupt). However, INTMASK GPIO register can still be
> used to mask the interrupt in question.

A-ha! But why are you registering a irqdomain entry for an interrupt
that cannot be used, hm?

> So when we specify handle_bad_irq as handler the IRQ core thinks the
> handler is being uninstalled and masks the interrupt.

You should not register any handle for it.

In fact, IMO the irqdomain should reject it being mapped, as it is not
for the kernel to use.

Check:
drivers/irqchip/irq-vic.c

We supply a u32 to the driver from the device tree named "valid-mask".
This has bits set to 1 for the valid (to be mapped) IRQs and zero
for those we may not touch.

So in our vic_irqdomain_map() call we have:

/* Skip invalid IRQs, only register handlers for the real ones */
if (!(v->valid_sources & (1 << hwirq)))
        return -EPERM;

So it can never be mapped.

Also notice:

/* create an IRQ mapping for each valid IRQ */
for (i = 0; i < fls(valid_sources); i++)
        if (valid_sources & (1 << i))
                irq_create_mapping(v->domain, i);

So we only create mappings where there are valid IRQs,
skipping over any "holes" in the mapping.

We should do something like this.

To make this play nicely with gpiolib_irqchip_add() I suggest
adding a bitmap valid_mask to struct gpio_chip() in
include/linux/gpio/driver.h
inside the CONFIG_GPIOLIB_IRQCHIP

I guess

u32 bitmap[MAX_IRQS_FOR_A_GPIO_CHIP];

Then augment the generic GPIO IRQCHIP helpers in
drivers/gpio/gpiolib.c per above so that the invalid
IRQs can't be mapped.

The driver would just:

/* Mark line N as invalid: used by BIOS */
set_bit(&chip->valid_mask, N);

Before calling gpiolib_irqchip_add().

This has the downside of roofing the number of lines that can
be flagged as valid/invalid, but I'm open to more advanced
ideas on this, but the check needs to be fast in the irqdomain.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Tue, Sep 13, 2016 at 02:22:25PM +0200, Linus Walleij wrote:
> On Tue, Sep 13, 2016 at 11:33 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Sep 13, 2016 at 11:18:49AM +0200, Linus Walleij wrote:
> >> On Mon, Sep 12, 2016 at 3:11 PM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > On Mon, Sep 12, 2016 at 09:04:44PM +0800, Phidias Chiang wrote:
> >> >> On Mon, Sep 12, 2016 at 12:04:01PM +0300, Mika Westerberg wrote:
> >> >> >
> >> >> > OK, I see what is going on now. When I changed handle_simple_irq to
> >> >> > handle_bad_irq, the IRQ core in __irq_do_set_handler() thinks the
> >> >> > handler is uninstalled and masks the line.
> >> >> >
> >> >> > If you change handle_bad_irq to handle_simple_irq, in call to
> >> >> > gpiochip_irqchip_add(), does it work then?
> >> >>
> >> >> Yes it does :), thank you for the support!
> >> >
> >> > Thanks for testing.
> >> >
> >> > So we need to use handle_simple_irq here instead.
> >> >
> >> > Linus, do you see any problems with that?
> >>
> >> I need to see the patch in its context with a commit message,
> >> I can't figure it out from the thread.
> >>
> >> handle_simple_irq() is for something generic not level- or
> >> edge-triggered. If you support specific triggers only, it
> >> should not be used.
> >>
> >> Nominally assigning handle_bad_irq() until a specific
> >> edge or level is requested is the right thing to do, since
> >> the IRQ is really not configured for anything at all and
> >> hence has undefined behaviour.
> >
> > For Cherryview/Braswell some interrupts are actually configured by the
> > BIOS but they are routed directly to the I/O-APIC and are supposed to be
> > handled without involvement of the GPIO driver (an example of this is
> > the ACPI SCI interrupt). However, INTMASK GPIO register can still be
> > used to mask the interrupt in question.
> 
> A-ha! But why are you registering a irqdomain entry for an interrupt
> that cannot be used, hm?

Unfortunately there is no way to figure out from the hardware (or
firmware) whether the interrupt is supposed to be used by the GPIO
driver or something else.

> > So when we specify handle_bad_irq as handler the IRQ core thinks the
> > handler is being uninstalled and masks the interrupt.
> 
> You should not register any handle for it.
> 
> In fact, IMO the irqdomain should reject it being mapped, as it is not
> for the kernel to use.
> 
> Check:
> drivers/irqchip/irq-vic.c
> 
> We supply a u32 to the driver from the device tree named "valid-mask".
> This has bits set to 1 for the valid (to be mapped) IRQs and zero
> for those we may not touch.
> 
> So in our vic_irqdomain_map() call we have:
> 
> /* Skip invalid IRQs, only register handlers for the real ones */
> if (!(v->valid_sources & (1 << hwirq)))
>         return -EPERM;
> 
> So it can never be mapped.
> 
> Also notice:
> 
> /* create an IRQ mapping for each valid IRQ */
> for (i = 0; i < fls(valid_sources); i++)
>         if (valid_sources & (1 << i))
>                 irq_create_mapping(v->domain, i);
> 
> So we only create mappings where there are valid IRQs,
> skipping over any "holes" in the mapping.
> 
> We should do something like this.
> 
> To make this play nicely with gpiolib_irqchip_add() I suggest
> adding a bitmap valid_mask to struct gpio_chip() in
> include/linux/gpio/driver.h
> inside the CONFIG_GPIOLIB_IRQCHIP
> 
> I guess
> 
> u32 bitmap[MAX_IRQS_FOR_A_GPIO_CHIP];
> 
> Then augment the generic GPIO IRQCHIP helpers in
> drivers/gpio/gpiolib.c per above so that the invalid
> IRQs can't be mapped.
> 
> The driver would just:
> 
> /* Mark line N as invalid: used by BIOS */
> set_bit(&chip->valid_mask, N);

Otherwise this probably works but it forces us to hardcode these
"special" lines in the driver and I would like to avoid that if
possible.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Linus Walleij]

On Tue, Sep 13, 2016 at 2:52 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> [Me]
>> A-ha! But why are you registering a irqdomain entry for an interrupt
>> that cannot be used, hm?
>
> Unfortunately there is no way to figure out from the hardware (or
> firmware) whether the interrupt is supposed to be used by the GPIO
> driver or something else.

So the fact that we kept it in valid-mask in the DT was a hint: it is
part of the hardware description.

Isn't this (a list of what IRQs are reserved by BIOS) by sheer logic
something that ACPI should provide?

Or is this one of those "well we could alter ACPI tables but we can't
because they already shipped so we just can't so now we need to
hack around it"?

Letting Linux map an interrupt it cannot access and then papering it
over by using handle_simple_irq() just feels wrong to me.

I would argue for associating the mask of BIOS-reserved IRQs with
something in ACPI and implement the mentioned scheme to avoid
even mapping them seems most logical.

If we have to use handle_simple_irq() by default on all I prefer to put
in a very fat comment of the type:

/*
 * HACK HACK HACK HACK HACK HACK HACK HACK HACK HACK
 *
 * Some interrupts are BIOS-reserved but we don't know which ones!
 * So we anyway map them and assign the handle_simple_irq() handle
 * to them, leaving them unmasked, pretending they can be used, and
 * pray no-one will accidentally use these GPIO IRQs.
 *
 * HACK HACK HACK HACK HACK HACK HACK HACK HACK HACK
  */

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Tue, Sep 13, 2016 at 10:57:31PM +0200, Linus Walleij wrote:
> On Tue, Sep 13, 2016 at 2:52 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > [Me]
> >> A-ha! But why are you registering a irqdomain entry for an interrupt
> >> that cannot be used, hm?
> >
> > Unfortunately there is no way to figure out from the hardware (or
> > firmware) whether the interrupt is supposed to be used by the GPIO
> > driver or something else.
> 
> So the fact that we kept it in valid-mask in the DT was a hint: it is
> part of the hardware description.
> 
> Isn't this (a list of what IRQs are reserved by BIOS) by sheer logic
> something that ACPI should provide?
> 
> Or is this one of those "well we could alter ACPI tables but we can't
> because they already shipped so we just can't so now we need to
> hack around it"?

Isn't it always the case? ;-)

Once the hardware enters stores the firmware cannot be changed anymore
and we get all the fun working around problems in the OS.

> Letting Linux map an interrupt it cannot access and then papering it
> over by using handle_simple_irq() just feels wrong to me.
> 
> I would argue for associating the mask of BIOS-reserved IRQs with
> something in ACPI and implement the mentioned scheme to avoid
> even mapping them seems most logical.

I'm going to re-read the hardware spec and see if there is anything we
can do about this. The newer hardware (Skylake, Broxton) has a bit that
tells the IRQ is routed directly to I/O-APIC but unfortunately Braswell
misses that. There may be something else, though.

> If we have to use handle_simple_irq() by default on all I prefer to put
> in a very fat comment of the type:
> 
> /*
>  * HACK HACK HACK HACK HACK HACK HACK HACK HACK HACK
>  *
>  * Some interrupts are BIOS-reserved but we don't know which ones!
>  * So we anyway map them and assign the handle_simple_irq() handle
>  * to them, leaving them unmasked, pretending they can be used, and
>  * pray no-one will accidentally use these GPIO IRQs.
>  *
>  * HACK HACK HACK HACK HACK HACK HACK HACK HACK HACK
>   */

OK, got it.

Let me try to come up with a solution that both works and does not
involve using handle_simple_irq.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Linus Walleij]

On Wed, Sep 14, 2016 at 10:26 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Sep 13, 2016 at 10:57:31PM +0200, Linus Walleij wrote:

>> Isn't this (a list of what IRQs are reserved by BIOS) by sheer logic
>> something that ACPI should provide?
>>
>> Or is this one of those "well we could alter ACPI tables but we can't
>> because they already shipped so we just can't so now we need to
>> hack around it"?
>
> Isn't it always the case? ;-)
>
> Once the hardware enters stores the firmware cannot be changed anymore
> and we get all the fun working around problems in the OS.

To me this is just a big proof that the ACPI design-by-committee isn't
working. With Device Tree we review bindings and drivers together
and then we tend to not miss stuff like this as much.

But I realize as soon as I say that someone will pull out a counter-example
of stupid DT bindings used in the wild and make me look stupid.

>> Letting Linux map an interrupt it cannot access and then papering it
>> over by using handle_simple_irq() just feels wrong to me.
>>
>> I would argue for associating the mask of BIOS-reserved IRQs with
>> something in ACPI and implement the mentioned scheme to avoid
>> even mapping them seems most logical.
>
> I'm going to re-read the hardware spec and see if there is anything we
> can do about this. The newer hardware (Skylake, Broxton) has a bit that
> tells the IRQ is routed directly to I/O-APIC but unfortunately Braswell
> misses that. There may be something else, though.

So as far as we can determine:

(A) we are running on Braswell and
(B) we are probing this driver

we can conclude that

(C) IRQs A,B,C are reserved by BIOS?

That sounds doable?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Wed, Sep 14, 2016 at 02:46:01PM +0200, Linus Walleij wrote:
> > I'm going to re-read the hardware spec and see if there is anything we
> > can do about this. The newer hardware (Skylake, Broxton) has a bit that
> > tells the IRQ is routed directly to I/O-APIC but unfortunately Braswell
> > misses that. There may be something else, though.
> 
> So as far as we can determine:
> 
> (A) we are running on Braswell and
> (B) we are probing this driver
> 
> we can conclude that
> 
> (C) IRQs A,B,C are reserved by BIOS?
> 
> That sounds doable?

Yes, it's doable but that requires some hard coding in the driver :-/

I'll look into it.

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Linus Walleij]

On Wed, Sep 14, 2016 at 5:12 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Sep 14, 2016 at 02:46:01PM +0200, Linus Walleij wrote:
>> > I'm going to re-read the hardware spec and see if there is anything we
>> > can do about this. The newer hardware (Skylake, Broxton) has a bit that
>> > tells the IRQ is routed directly to I/O-APIC but unfortunately Braswell
>> > misses that. There may be something else, though.
>>
>> So as far as we can determine:
>>
>> (A) we are running on Braswell and
>> (B) we are probing this driver
>>
>> we can conclude that
>>
>> (C) IRQs A,B,C are reserved by BIOS?
>>
>> That sounds doable?
>
> Yes, it's doable but that requires some hard coding in the driver :-/

>From my point of view that is the lesser of two evils.

We only have hard-coding (syntactic) madness over having
behaviour-dependent (semantic) madness.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe

[Mika Westerberg]

On Thu, Sep 15, 2016 at 02:39:47PM +0200, Linus Walleij wrote:
> On Wed, Sep 14, 2016 at 5:12 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Sep 14, 2016 at 02:46:01PM +0200, Linus Walleij wrote:
> >> > I'm going to re-read the hardware spec and see if there is anything we
> >> > can do about this. The newer hardware (Skylake, Broxton) has a bit that
> >> > tells the IRQ is routed directly to I/O-APIC but unfortunately Braswell
> >> > misses that. There may be something else, though.
> >>
> >> So as far as we can determine:
> >>
> >> (A) we are running on Braswell and
> >> (B) we are probing this driver
> >>
> >> we can conclude that
> >>
> >> (C) IRQs A,B,C are reserved by BIOS?
> >>
> >> That sounds doable?
> >
> > Yes, it's doable but that requires some hard coding in the driver :-/
> 
> >From my point of view that is the lesser of two evils.
> 
> We only have hard-coding (syntactic) madness over having
> behaviour-dependent (semantic) madness.

I re-read the hardware spec now and it occured to me that for north and
southwest community, only the first 8 IRQs can be used as interrupts
(all GPIOs which have IntSel value < 8). Rest can only trigger GPEs
which are used for EC events.

I'll submit patches shortly using this information and valid_mask as you
suggested.

[PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain

[Mika Westerberg]

When using GPIO irqchip helpers to setup irqchip for a gpiolib based
driver, it is not possible to select which GPIOs to add to the IRQ domain.
Instead it just adds all GPIOs which is not always desired. For example
there might be GPIOs that for some reason just cannot be used as interrupts
at all.

To make this possible we add valid_mask to each gpio_chip and by default
assume all GPIOs can be used as interrupts. Drivers can then tune this
using clear_bit() or similar before they call gpiochip_irqchip_add().

Suggested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib.c      | 32 +++++++++++++++++++++++++++++---
 include/linux/gpio/driver.h |  1 +
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 53ff25ac66d8..d84c23b47f44 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
 		if (status)
 			goto err_remove_chip;
 	}
+
+#ifdef CONFIG_GPIOLIB_IRQCHIP
+	chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long),
+				   GFP_KERNEL);
+	if (!chip->valid_mask)
+		return -ENOMEM;
+
+	/* All GPIOs are valid interrupt sources by default */
+	for (i = 0; i < chip->ngpio; i++)
+		set_bit(i, chip->valid_mask);
+#endif
+
 	return 0;
 
 err_remove_chip:
@@ -1442,9 +1454,12 @@ void gpiochip_set_chained_irqchip(struct gpio_chip *gpiochip,
 	}
 
 	/* Set the parent IRQ for all affected IRQs */
-	for (offset = 0; offset < gpiochip->ngpio; offset++)
+	for (offset = 0; offset < gpiochip->ngpio; offset++) {
+		if (!test_bit(offset, gpiochip->valid_mask))
+			continue;
 		irq_set_parent(irq_find_mapping(gpiochip->irqdomain, offset),
 			       parent_irq);
+	}
 }
 EXPORT_SYMBOL_GPL(gpiochip_set_chained_irqchip);
 
@@ -1551,9 +1566,12 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 
 	/* Remove all IRQ mappings and delete the domain */
 	if (gpiochip->irqdomain) {
-		for (offset = 0; offset < gpiochip->ngpio; offset++)
+		for (offset = 0; offset < gpiochip->ngpio; offset++) {
+			if (!test_bit(offset, gpiochip->valid_mask))
+				continue;
 			irq_dispose_mapping(
 				irq_find_mapping(gpiochip->irqdomain, offset));
+		}
 		irq_domain_remove(gpiochip->irqdomain);
 	}
 
@@ -1562,6 +1580,9 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
 		gpiochip->irqchip->irq_release_resources = NULL;
 		gpiochip->irqchip = NULL;
 	}
+
+	kfree(gpiochip->valid_mask);
+	gpiochip->valid_mask = NULL;
 }
 
 /**
@@ -1597,6 +1618,7 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
 			  struct lock_class_key *lock_key)
 {
 	struct device_node *of_node;
+	bool irq_base_set = false;
 	unsigned int offset;
 	unsigned irq_base = 0;
 
@@ -1646,13 +1668,17 @@ int _gpiochip_irqchip_add(struct gpio_chip *gpiochip,
 	 * necessary to allocate descriptors for all IRQs.
 	 */
 	for (offset = 0; offset < gpiochip->ngpio; offset++) {
+		if (!test_bit(offset, gpiochip->valid_mask))
+			continue;
 		irq_base = irq_create_mapping(gpiochip->irqdomain, offset);
-		if (offset == 0)
+		if (!irq_base_set) {
 			/*
 			 * Store the base into the gpiochip to be used when
 			 * unmapping the irqs.
 			 */
 			gpiochip->irq_base = irq_base;
+			irq_base_set = true;
+		}
 	}
 
 	acpi_gpiochip_request_interrupts(gpiochip);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 50882e09289b..8a7430230562 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -191,6 +191,7 @@ struct gpio_chip {
 	unsigned int		irq_default_type;
 	int			irq_parent;
 	struct lock_class_key	*lock_key;
+	unsigned long		*valid_mask;
 #endif
 
 #if defined(CONFIG_OF_GPIO)
-- 
2.9.3

[PATCH 2/2] pinctrl: cherryview: Do not add all southwest and north GPIOs to IRQ domain

[Mika Westerberg]

It turns out that for north and southwest communities, they can only
generate GPIO interrupts for lower 8 interrupts (IntSel value). The upper
part (8-15) can only generate GPEs (General Purpose Events).

Now the reason why EC events such as pressing hotkeys does not work if we
mask all the interrupts is that in order to generate either interrupts or
GPEs the INTMASK register must have that particular interrupt unmasked. In
case of GPEs the CPU does not trigger normal interrupt (and thus the GPIO
driver does not see it) but instead it causes SCI (System Control
Interrupt) to be triggered with the GPE in question set.

To make this all work as expected we only add those GPIOs to the IRQ domain
that can actually generate interrupts (IntSel value 0-7) and skip others.

Reported-by: Phidias Chiang <phidias.chiang@canonical.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Hi Phidias and Anisse,

Can you try this series out? It should finally fix the EC event issue(s)
you have reported.

 drivers/pinctrl/intel/pinctrl-cherryview.c | 31 ++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 0fe8fad25e4d..717ef1b00adb 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -134,6 +134,7 @@ struct chv_gpio_pinrange {
  * @gpio_ranges: An array of GPIO ranges in this community
  * @ngpio_ranges: Number of GPIO ranges
  * @ngpios: Total number of GPIOs in this community
+ * @nirqs: Total number of IRQs this community can generate
  */
 struct chv_community {
 	const char *uid;
@@ -146,6 +147,7 @@ struct chv_community {
 	const struct chv_gpio_pinrange *gpio_ranges;
 	size_t ngpio_ranges;
 	size_t ngpios;
+	size_t nirqs;
 };
 
 struct chv_pin_context {
@@ -396,6 +398,12 @@ static const struct chv_community southwest_community = {
 	.gpio_ranges = southwest_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(southwest_gpio_ranges),
 	.ngpios = ARRAY_SIZE(southwest_pins),
+	/*
+	 * Southwest community can benerate GPIO interrupts only for the
+	 * first 8 interrupts. The upper half (8-15) can only be used to
+	 * trigger GPEs.
+	 */
+	.nirqs = 8,
 };
 
 static const struct pinctrl_pin_desc north_pins[] = {
@@ -479,6 +487,12 @@ static const struct chv_community north_community = {
 	.gpio_ranges = north_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(north_gpio_ranges),
 	.ngpios = ARRAY_SIZE(north_pins),
+	/*
+	 * North community can benerate GPIO interrupts only for the first
+	 * 8 interrupts. The upper half (8-15) can only be used to trigger
+	 * GPEs.
+	 */
+	.nirqs = 8,
 };
 
 static const struct pinctrl_pin_desc east_pins[] = {
@@ -521,6 +535,7 @@ static const struct chv_community east_community = {
 	.gpio_ranges = east_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(east_gpio_ranges),
 	.ngpios = ARRAY_SIZE(east_pins),
+	.nirqs = 16,
 };
 
 static const struct pinctrl_pin_desc southeast_pins[] = {
@@ -646,6 +661,7 @@ static const struct chv_community southeast_community = {
 	.gpio_ranges = southeast_gpio_ranges,
 	.ngpio_ranges = ARRAY_SIZE(southeast_gpio_ranges),
 	.ngpios = ARRAY_SIZE(southeast_pins),
+	.nirqs = 16,
 };
 
 static const struct chv_community *chv_communities[] = {
@@ -1539,6 +1555,21 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		offset += range->npins;
 	}
 
+	/* Do not add GPIOs that can only generate GPEs to the IRQ domain */
+	for (i = 0; i < pctrl->community->npins; i++) {
+		const struct pinctrl_pin_desc *desc;
+		u32 intsel;
+
+		desc = &pctrl->community->pins[i];
+
+		intsel = readl(chv_padreg(pctrl, desc->number, CHV_PADCTRL0));
+		intsel &= CHV_PADCTRL0_INTSEL_MASK;
+		intsel >>= CHV_PADCTRL0_INTSEL_SHIFT;
+
+		if (intsel >= pctrl->community->nirqs)
+			clear_bit(i, chip->valid_mask);
+	}
+
 	/* Clear all interrupts */
 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
 
-- 
2.9.3

Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain

[Marc Zyngier]

Mika,

On 15/09/16 16:52, Mika Westerberg wrote:
> When using GPIO irqchip helpers to setup irqchip for a gpiolib based
> driver, it is not possible to select which GPIOs to add to the IRQ domain.
> Instead it just adds all GPIOs which is not always desired. For example
> there might be GPIOs that for some reason just cannot be used as interrupts
> at all.
> 
> To make this possible we add valid_mask to each gpio_chip and by default
> assume all GPIOs can be used as interrupts. Drivers can then tune this
> using clear_bit() or similar before they call gpiochip_irqchip_add().
> 
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib.c      | 32 +++++++++++++++++++++++++++++---
>  include/linux/gpio/driver.h |  1 +
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 53ff25ac66d8..d84c23b47f44 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
>  		if (status)
>  			goto err_remove_chip;
>  	}
> +
> +#ifdef CONFIG_GPIOLIB_IRQCHIP
> +	chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long),
> +				   GFP_KERNEL);

Do we really want to make this a mandatory thing? In most cases, I'd
expect this valid_mask to have all bits set, so you might as well not
allocate it at all in that case (and only allocate it if you actually
need it).

> +	if (!chip->valid_mask)
> +		return -ENOMEM;

You really want to revise your error handling here.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain

[Mika Westerberg]

On Thu, Sep 15, 2016 at 05:07:58PM +0100, Marc Zyngier wrote:
> Mika,
> 
> On 15/09/16 16:52, Mika Westerberg wrote:
> > When using GPIO irqchip helpers to setup irqchip for a gpiolib based
> > driver, it is not possible to select which GPIOs to add to the IRQ domain.
> > Instead it just adds all GPIOs which is not always desired. For example
> > there might be GPIOs that for some reason just cannot be used as interrupts
> > at all.
> > 
> > To make this possible we add valid_mask to each gpio_chip and by default
> > assume all GPIOs can be used as interrupts. Drivers can then tune this
> > using clear_bit() or similar before they call gpiochip_irqchip_add().
> > 
> > Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/gpio/gpiolib.c      | 32 +++++++++++++++++++++++++++++---
> >  include/linux/gpio/driver.h |  1 +
> >  2 files changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 53ff25ac66d8..d84c23b47f44 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
> >  		if (status)
> >  			goto err_remove_chip;
> >  	}
> > +
> > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > +	chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long),
> > +				   GFP_KERNEL);
> 
> Do we really want to make this a mandatory thing? In most cases, I'd
> expect this valid_mask to have all bits set, so you might as well not
> allocate it at all in that case (and only allocate it if you actually
> need it).

We can make drivers to allocate it and only if set use it in core.

> 
> > +	if (!chip->valid_mask)
> > +		return -ENOMEM;
> 
> You really want to revise your error handling here.

Oops, right. It should be:

	if (!chip->valid_mask) {
		status = -ENOMEM;
		goto err_remove_chip;
	}

Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain

[Thomas Gleixner]

On Thu, 15 Sep 2016, Mika Westerberg wrote:
> On Thu, Sep 15, 2016 at 05:07:58PM +0100, Marc Zyngier wrote:
> > Mika,
> > 
> > On 15/09/16 16:52, Mika Westerberg wrote:
> > > When using GPIO irqchip helpers to setup irqchip for a gpiolib based
> > > driver, it is not possible to select which GPIOs to add to the IRQ domain.
> > > Instead it just adds all GPIOs which is not always desired. For example
> > > there might be GPIOs that for some reason just cannot be used as interrupts
> > > at all.
> > > 
> > > To make this possible we add valid_mask to each gpio_chip and by default
> > > assume all GPIOs can be used as interrupts. Drivers can then tune this
> > > using clear_bit() or similar before they call gpiochip_irqchip_add().
> > > 
> > > Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/gpio/gpiolib.c      | 32 +++++++++++++++++++++++++++++---
> > >  include/linux/gpio/driver.h |  1 +
> > >  2 files changed, 30 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 53ff25ac66d8..d84c23b47f44 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1186,6 +1186,18 @@ int gpiochip_add_data(struct gpio_chip *chip, void *data)
> > >  		if (status)
> > >  			goto err_remove_chip;
> > >  	}
> > > +
> > > +#ifdef CONFIG_GPIOLIB_IRQCHIP
> > > +	chip->valid_mask = kcalloc(BITS_TO_LONGS(chip->ngpio), sizeof(long),
> > > +				   GFP_KERNEL);
> > 
> > Do we really want to make this a mandatory thing? In most cases, I'd
> > expect this valid_mask to have all bits set, so you might as well not
> > allocate it at all in that case (and only allocate it if you actually
> > need it).
> 
> We can make drivers to allocate it and only if set use it in core.

Make it a flag in gpio_chip which allocates it here when requested so we
don't end up with driver writers getting the alloc/free/error handling
wrong over and over.

Thanks,

	tglx

Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain

[Linus Walleij]

On Thu, Sep 15, 2016 at 5:52 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> When using GPIO irqchip helpers to setup irqchip for a gpiolib based
> driver, it is not possible to select which GPIOs to add to the IRQ domain.
> Instead it just adds all GPIOs which is not always desired. For example
> there might be GPIOs that for some reason just cannot be used as interrupts
> at all.
>
> To make this possible we add valid_mask to each gpio_chip and by default
> assume all GPIOs can be used as interrupts. Drivers can then tune this
> using clear_bit() or similar before they call gpiochip_irqchip_add().
>
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Awesome, nice infrastructure.
Will apply once tglx and Marc's comments are fixed.

Yours,
Linus Walleij