[PATCH] pinctrl: intel: Configure pin as GPIO input when used directly through irqchip

[PATCH] pinctrl: intel: Configure pin as GPIO input when used directly through irqchip

[Mika Westerberg]

If a pin is used directly through irqchip without requesting it first as
GPIO, it might be in wrong mode (for example input buffer disabled). This
means the user may never get any interrupts.

Fix this by configuring the pin as GPIO input when its type is first set in
irq_set_type().

Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Since we probably need to do this for cherryview and baytrail pinctrl
drivers as well, I'm thinking is this something that the GPIO core could do
automatically?

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 01443762e570..a1a5e2a77f9e 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -353,6 +353,23 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
 	return 0;
 }
 
+/* Called with pctrl->lock held */
+static void intel_gpio_set_gpio_mode(struct intel_pinctrl *pctrl,
+				     void __iomem *padcfg0)
+{
+	u32 value;
+
+	/* Put the pad into GPIO mode */
+	value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
+	/* Disable SCI/SMI/NMI generation */
+	value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
+	value &= ~(PADCFG0_GPIROUTSMI | PADCFG0_GPIROUTNMI);
+	/* Disable TX buffer and enable RX (this will be input) */
+	value &= ~PADCFG0_GPIORXDIS;
+	value |= PADCFG0_GPIOTXDIS;
+	writel(value, padcfg0);
+}
+
 static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 				     struct pinctrl_gpio_range *range,
 				     unsigned pin)
@@ -360,29 +377,26 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	void __iomem *padcfg0;
 	unsigned long flags;
-	u32 value;
+	int ret = 0;
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	if (!intel_pad_usable(pctrl, pin)) {
-		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-		return -EBUSY;
+		ret = -EBUSY;
+		goto out;
 	}
 
 	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
-	/* Put the pad into GPIO mode */
-	value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
-	/* Disable SCI/SMI/NMI generation */
-	value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
-	value &= ~(PADCFG0_GPIROUTSMI | PADCFG0_GPIROUTNMI);
-	/* Disable TX buffer and enable RX (this will be input) */
-	value &= ~PADCFG0_GPIORXDIS;
-	value |= PADCFG0_GPIOTXDIS;
-	writel(value, padcfg0);
+	if (!padcfg0) {
+		ret = -EINVAL;
+		goto out;
+	}
 
+	/* Set to GPIO input */
+	intel_gpio_set_gpio_mode(pctrl, padcfg0);
+out:
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-
-	return 0;
+	return ret;
 }
 
 static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
@@ -762,8 +776,10 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
-	value = readl(reg);
+	/* Make sure the pin is GPIO input */
+	intel_gpio_set_gpio_mode(pctrl, reg);
 
+	value = readl(reg);
 	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
 
 	if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) {
-- 
2.9.3

Re: [PATCH] pinctrl: intel: Configure pin as GPIO input when used directly through irqchip

[Jarkko Nikula]

On 11/09/2016 01:22 PM, Mika Westerberg wrote:
> If a pin is used directly through irqchip without requesting it first as
> GPIO, it might be in wrong mode (for example input buffer disabled). This
> means the user may never get any interrupts.
>
> Fix this by configuring the pin as GPIO input when its type is first set in
> irq_set_type().
>
> Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Since we probably need to do this for cherryview and baytrail pinctrl
> drivers as well, I'm thinking is this something that the GPIO core could do
> automatically?
>
>  drivers/pinctrl/intel/pinctrl-intel.c | 46 +++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
>
Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH] pinctrl: intel: Configure pin as GPIO input when used directly through irqchip

[Linus Walleij]

On Wed, Nov 9, 2016 at 12:22 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> If a pin is used directly through irqchip without requesting it first as
> GPIO, it might be in wrong mode (for example input buffer disabled). This
> means the user may never get any interrupts.
>
> Fix this by configuring the pin as GPIO input when its type is first set in
> irq_set_type().
>
> Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Since we probably need to do this for cherryview and baytrail pinctrl
> drivers as well, I'm thinking is this something that the GPIO core could do
> automatically?

No it is a property of the irqchip core to set up the lines it needs
to use with hardware-specific code. It is a part of the contract that
the irqchip should always set up all the hardware it needs.

>From Documentation/gpio/driver.txt:

-----8<--------------------------8<--------------------------
It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
irq_chip are orthogonal, and offering their services independent of each
other.

gpiod_to_irq() is just a convenience function to figure out the IRQ for a
certain GPIO line and should not be relied upon to have been called before
the IRQ is used.

So always prepare the hardware and make it ready for action in respective
callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
been called first.
(...)
-----8<--------------------------8<--------------------------

And I agree you should check the other drivers for the same problem.

> +/* Called with pctrl->lock held */
> +static void intel_gpio_set_gpio_mode(struct intel_pinctrl *pctrl,
> +                                    void __iomem *padcfg0)
> +{
> +       u32 value;
> +
> +       /* Put the pad into GPIO mode */
> +       value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
> +       /* Disable SCI/SMI/NMI generation */
> +       value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
> +       value &= ~(PADCFG0_GPIROUTSMI | PADCFG0_GPIROUTNMI);
> +       /* Disable TX buffer and enable RX (this will be input) */
> +       value &= ~PADCFG0_GPIORXDIS;
> +       value |= PADCFG0_GPIOTXDIS;
> +       writel(value, padcfg0);
> +}

Looks good.

> @@ -762,8 +776,10 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
>
>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> -       value = readl(reg);
> +       /* Make sure the pin is GPIO input */

You already KNOW it is set as input, because you are implementing
.get_direction() in your gpiochip and the gpio descriptor thus contains
the (cached) direction setting.

When the GPIOLIB_IRQCHIP calles its request resources hook
it uses the gpiochip_lock_as_irq() call which verifies that the
line is indeed an input. Else it would deny it to be used as an IRQ.

> +       intel_gpio_set_gpio_mode(pctrl, reg);
>
> +       value = readl(reg);
>         value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);

Why is this setting done in .irq_set_type() rather than .irq_enable()
from the irqchip? I'd say implement .irq_enable() with just this call to
intel_gpio_set_gpio_mode() in it.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: intel: Configure pin as GPIO input when used directly through irqchip

[Mika Westerberg]

On Tue, Nov 15, 2016 at 09:40:27AM +0100, Linus Walleij wrote:
> On Wed, Nov 9, 2016 at 12:22 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > If a pin is used directly through irqchip without requesting it first as
> > GPIO, it might be in wrong mode (for example input buffer disabled). This
> > means the user may never get any interrupts.
> >
> > Fix this by configuring the pin as GPIO input when its type is first set in
> > irq_set_type().
> >
> > Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > Since we probably need to do this for cherryview and baytrail pinctrl
> > drivers as well, I'm thinking is this something that the GPIO core could do
> > automatically?
> 
> No it is a property of the irqchip core to set up the lines it needs
> to use with hardware-specific code. It is a part of the contract that
> the irqchip should always set up all the hardware it needs.
> 
> >From Documentation/gpio/driver.txt:
> 
> -----8<--------------------------8<--------------------------
> It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
> if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
> irq_chip are orthogonal, and offering their services independent of each
> other.
> 
> gpiod_to_irq() is just a convenience function to figure out the IRQ for a
> certain GPIO line and should not be relied upon to have been called before
> the IRQ is used.
> 
> So always prepare the hardware and make it ready for action in respective
> callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
> been called first.
> (...)
> -----8<--------------------------8<--------------------------

OK, thanks for the detailed description.

> And I agree you should check the other drivers for the same problem.
> 
> > +/* Called with pctrl->lock held */
> > +static void intel_gpio_set_gpio_mode(struct intel_pinctrl *pctrl,
> > +                                    void __iomem *padcfg0)
> > +{
> > +       u32 value;
> > +
> > +       /* Put the pad into GPIO mode */
> > +       value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
> > +       /* Disable SCI/SMI/NMI generation */
> > +       value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
> > +       value &= ~(PADCFG0_GPIROUTSMI | PADCFG0_GPIROUTNMI);
> > +       /* Disable TX buffer and enable RX (this will be input) */
> > +       value &= ~PADCFG0_GPIORXDIS;
> > +       value |= PADCFG0_GPIOTXDIS;
> > +       writel(value, padcfg0);
> > +}
> 
> Looks good.
> 
> > @@ -762,8 +776,10 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
> >
> >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > -       value = readl(reg);
> > +       /* Make sure the pin is GPIO input */
> 
> You already KNOW it is set as input, because you are implementing
> .get_direction() in your gpiochip and the gpio descriptor thus contains
> the (cached) direction setting.
> 
> When the GPIOLIB_IRQCHIP calles its request resources hook
> it uses the gpiochip_lock_as_irq() call which verifies that the
> line is indeed an input. Else it would deny it to be used as an IRQ.

Actually it is not always input. The problem Jarkko reported happens
because the pin is actually configured as output by the BIOS originally
(or left untouched). The pin is wired to a pin header on Joule where the
BIOS does not know in advance what will be connected to it.

> > +       intel_gpio_set_gpio_mode(pctrl, reg);
> >
> > +       value = readl(reg);
> >         value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
> 
> Why is this setting done in .irq_set_type() rather than .irq_enable()
> from the irqchip? I'd say implement .irq_enable() with just this call to
> intel_gpio_set_gpio_mode() in it.

OK, I'll move it to .irq_enable() instead.

Re: [PATCH] pinctrl: intel: Configure pin as GPIO input when used directly through irqchip

[Mika Westerberg]

On Tue, Nov 15, 2016 at 01:23:04PM +0200, Mika Westerberg wrote:
> On Tue, Nov 15, 2016 at 09:40:27AM +0100, Linus Walleij wrote:
> > On Wed, Nov 9, 2016 at 12:22 PM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > 
> > > If a pin is used directly through irqchip without requesting it first as
> > > GPIO, it might be in wrong mode (for example input buffer disabled). This
> > > means the user may never get any interrupts.
> > >
> > > Fix this by configuring the pin as GPIO input when its type is first set in
> > > irq_set_type().
> > >
> > > Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > > Since we probably need to do this for cherryview and baytrail pinctrl
> > > drivers as well, I'm thinking is this something that the GPIO core could do
> > > automatically?
> > 
> > No it is a property of the irqchip core to set up the lines it needs
> > to use with hardware-specific code. It is a part of the contract that
> > the irqchip should always set up all the hardware it needs.
> > 
> > >From Documentation/gpio/driver.txt:
> > 
> > -----8<--------------------------8<--------------------------
> > It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
> > if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
> > irq_chip are orthogonal, and offering their services independent of each
> > other.
> > 
> > gpiod_to_irq() is just a convenience function to figure out the IRQ for a
> > certain GPIO line and should not be relied upon to have been called before
> > the IRQ is used.
> > 
> > So always prepare the hardware and make it ready for action in respective
> > callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
> > been called first.
> > (...)
> > -----8<--------------------------8<--------------------------
> 
> OK, thanks for the detailed description.
> 
> > And I agree you should check the other drivers for the same problem.
> > 
> > > +/* Called with pctrl->lock held */
> > > +static void intel_gpio_set_gpio_mode(struct intel_pinctrl *pctrl,
> > > +                                    void __iomem *padcfg0)
> > > +{
> > > +       u32 value;
> > > +
> > > +       /* Put the pad into GPIO mode */
> > > +       value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
> > > +       /* Disable SCI/SMI/NMI generation */
> > > +       value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
> > > +       value &= ~(PADCFG0_GPIROUTSMI | PADCFG0_GPIROUTNMI);
> > > +       /* Disable TX buffer and enable RX (this will be input) */
> > > +       value &= ~PADCFG0_GPIORXDIS;
> > > +       value |= PADCFG0_GPIOTXDIS;
> > > +       writel(value, padcfg0);
> > > +}
> > 
> > Looks good.
> > 
> > > @@ -762,8 +776,10 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
> > >
> > >         raw_spin_lock_irqsave(&pctrl->lock, flags);
> > >
> > > -       value = readl(reg);
> > > +       /* Make sure the pin is GPIO input */
> > 
> > You already KNOW it is set as input, because you are implementing
> > .get_direction() in your gpiochip and the gpio descriptor thus contains
> > the (cached) direction setting.
> > 
> > When the GPIOLIB_IRQCHIP calles its request resources hook
> > it uses the gpiochip_lock_as_irq() call which verifies that the
> > line is indeed an input. Else it would deny it to be used as an IRQ.
> 
> Actually it is not always input. The problem Jarkko reported happens
> because the pin is actually configured as output by the BIOS originally
> (or left untouched). The pin is wired to a pin header on Joule where the
> BIOS does not know in advance what will be connected to it.

To follow up on this.

Since we use acpi_dev_gpio_irq_get() to get ACPI GpioInt() resource and
convert that to a Linux IRQ number, I think we can configure the pin
there as well (since we know that it is supposed to be used as GPIO
interrupt anyway).

Andy (Cc'd) is working on this among other ACPI GPIO related things.