Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1479032 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Phidias Chiang <phidias.chiang@canonical.com> - 2016-09-08 12:20 +0200
    Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-08 12:30 +0200
      Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Phidias Chiang <phidias.chiang@canonical.com> - 2016-09-08 18:30 +0200
        Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-09 08:30 +0200
          Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Phidias Chiang <phidias.chiang@canonical.com> - 2016-09-09 10:30 +0200
            Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-09 11:00 +0200
              Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-11 10:10 +0200
                Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Phidias Chiang <phidias.chiang@canonical.com> - 2016-09-12 09:00 +0200
                  Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-12 11:10 +0200
                    Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Phidias Chiang <phidias.chiang@canonical.com> - 2016-09-12 15:10 +0200
                      Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-12 15:30 +0200
                        Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Linus Walleij <linus.walleij@linaro.org> - 2016-09-13 11:20 +0200
                          Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-13 11:40 +0200
                            Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Linus Walleij <linus.walleij@linaro.org> - 2016-09-13 14:30 +0200
                              Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-13 15:00 +0200
                                Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Linus Walleij <linus.walleij@linaro.org> - 2016-09-13 23:00 +0200
                                  Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-14 10:30 +0200
                                    Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Linus Walleij <linus.walleij@linaro.org> - 2016-09-14 14:50 +0200
                                      Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-14 17:20 +0200
                                        Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Linus Walleij <linus.walleij@linaro.org> - 2016-09-15 14:50 +0200
                                          Re: [PATCH] pinctrl: cherryview: Do not mask all interrupts on probe Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-15 17:50 +0200
                                          [PATCH 2/2] pinctrl: cherryview: Do not add all southwest and north GPIOs to IRQ domain Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-15 18:00 +0200
                                          [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are added to IRQ domain Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-15 18:00 +0200
                                            Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are  added to IRQ domain Marc Zyngier <marc.zyngier@arm.com> - 2016-09-15 18:10 +0200
                                              Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are  added to IRQ domain Mika Westerberg <mika.westerberg@linux.intel.com> - 2016-09-15 20:20 +0200
                                                Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are  added to IRQ domain Thomas Gleixner <tglx@linutronix.de> - 2016-09-15 21:00 +0200
                                            Re: [PATCH 1/2] gpiolib: Add possibility to mask which GPIOs are  added to IRQ domain Linus Walleij <linus.walleij@linaro.org> - 2016-09-18 13:20 +0200

Page 1 of 2  [1] 2  Next page →

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

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.

[toc] | [next] | [standalone]

#1479047

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/

[toc] | [prev] | [next] | [standalone]

#1479366

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.

[toc] | [prev] | [next] | [standalone]

#1479679

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?

[toc] | [prev] | [next] | [standalone]

#1479737

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.

[toc] | [prev] | [next] | [standalone]

#1479763

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.

[toc] | [prev] | [next] | [standalone]

#1480752

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

[toc] | [prev] | [next] | [standalone]

#1480960

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

[toc] | [prev] | [next] | [standalone]

#1481063

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?

[toc] | [prev] | [next] | [standalone]

#1481243

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!

[toc] | [prev] | [next] | [standalone]

#1481277

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?

[toc] | [prev] | [next] | [standalone]

#1482289

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

[toc] | [prev] | [next] | [standalone]

#1482302

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.

[toc] | [prev] | [next] | [standalone]

#1482414

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

[toc] | [prev] | [next] | [standalone]

#1482441

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.

[toc] | [prev] | [next] | [standalone]

#1482775

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

[toc] | [prev] | [next] | [standalone]

#1483079

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.

[toc] | [prev] | [next] | [standalone]

#1483242

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

[toc] | [prev] | [next] | [standalone]

#1483427

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.

[toc] | [prev] | [next] | [standalone]

#1484128

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

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web