[PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output

[PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output

[eric.ernst]

From: Eric Ernst <eric.ernst@linux.intel.com>

For Baytrail, you should never set a GPIO set to direct_irq
to output mode.  When direct_irq_en is set for a GPIO, it is
tied directly to an APIC internally, and making the pad output
does not make any sense. Assert a WARN() in the event this happens.

Signed-off-by: Eric Ernst <eric.ernst@linux.intel.com>
---
 drivers/pinctrl/pinctrl-baytrail.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-baytrail.c b/drivers/pinctrl/pinctrl-baytrail.c
index e59983423991..677f79a9efca 100644
--- a/drivers/pinctrl/pinctrl-baytrail.c
+++ b/drivers/pinctrl/pinctrl-baytrail.c
@@ -47,6 +47,7 @@
 #define BYT_TRIG_POS		BIT(25)
 #define BYT_TRIG_LVL		BIT(24)
 #define BYT_PIN_MUX		0x07
+#define BYT_DIRECTIRQ		BIT(27)
 
 /* BYT_VAL_REG register bits */
 #define BYT_INPUT_EN		BIT(2)  /* 0: input enabled (active low)*/
@@ -256,19 +257,26 @@ static int byt_gpio_direction_output(struct gpio_chip *chip,
 				     unsigned gpio, int value)
 {
 	struct byt_gpio *vg = to_byt_gpio(chip);
-	void __iomem *reg = byt_gpio_reg(chip, gpio, BYT_VAL_REG);
+	void __iomem *conf_reg = byt_gpio_reg(chip, gpio, BYT_CONF0_REG);
+	void __iomem *value_reg = byt_gpio_reg(chip, gpio, BYT_VAL_REG);
 	unsigned long flags;
 	u32 reg_val;
 
 	spin_lock_irqsave(&vg->lock, flags);
 
-	reg_val = readl(reg) | BYT_DIR_MASK;
+	/* Before making any direction modifications, do a check if gpio
+	is set for direct IRQ.  On baytrail, setting GPIO to output does
+	not make sense, so let's at least warn the caller before they shoot
+	themselves in the foot */
+	WARN((readl(conf_reg) & BYT_DIRECTIRQ), "Potential Error: Setting GPIO with direct_irq_en to output");
+
+	reg_val = readl(value_reg) | BYT_DIR_MASK;
 	reg_val &= ~BYT_OUTPUT_EN;
 
 	if (value)
-		writel(reg_val | BYT_LEVEL, reg);
+		writel(reg_val | BYT_LEVEL, value_reg);
 	else
-		writel(reg_val & ~BYT_LEVEL, reg);
+		writel(reg_val & ~BYT_LEVEL, value_reg);
 
 	spin_unlock_irqrestore(&vg->lock, flags);
 
-- 
1.7.9.5

Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output

[Linus Walleij]

On Tue, May 27, 2014 at 9:26 PM,  <eric.ernst@linux.intel.com> wrote:

> From: Eric Ernst <eric.ernst@linux.intel.com>
>
> For Baytrail, you should never set a GPIO set to direct_irq
> to output mode.  When direct_irq_en is set for a GPIO, it is
> tied directly to an APIC internally, and making the pad output
> does not make any sense. Assert a WARN() in the event this happens.
>
> Signed-off-by: Eric Ernst <eric.ernst@linux.intel.com>

Can I get some ACK from the author's of this driver on Eric's patch?

Eric, you *are* aware of what the gpio_lock_as_irq() and
gpio_unlock_as_irq() in the .irq_request_resources are doing
right? Is this patch just some extra safety measure?

Yours,
Linus Walleij

Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output

[eric ernst]

On 14-05-29 06:44 AM, Linus Walleij wrote:
> On Tue, May 27, 2014 at 9:26 PM,  <eric.ernst@linux.intel.com> wrote:
>
>> From: Eric Ernst <eric.ernst@linux.intel.com>
>>
>> For Baytrail, you should never set a GPIO set to direct_irq
>> to output mode.  When direct_irq_en is set for a GPIO, it is
>> tied directly to an APIC internally, and making the pad output
>> does not make any sense. Assert a WARN() in the event this happens.
>>
>> Signed-off-by: Eric Ernst <eric.ernst@linux.intel.com>
> Can I get some ACK from the author's of this driver on Eric's patch?
>
> Eric, you *are* aware of what the gpio_lock_as_irq() and
> gpio_unlock_as_irq() in the .irq_request_resources are doing
> right? Is this patch just some extra safety measure?
>
> Yours,
> Linus Walleij
Linus, thanks for the feedback.

I do see the usage for gpio_lock_as_irq() and gpio_unlock_as_irq(), 
though I'm not sure if this would help me in this scenario.  I am adding 
an extra safety measure, as an attempt to check exactly how the GPIO pin 
was configured, possibly before kernel, before changing the direction to 
output.

In my situation, a device attached via GPIO will act as an IRQ, but also 
toggled as output during setup in order to communicate with the device 
(ugly device, i know).  Specifically for this pincntrl device on 
Baytrail, we need to make sure that if the pin were ever to be used for 
IO, that direct IRQ is not set in its config register (ie - don't handle 
the IRQ via the APIC).  This check and WARN is to look for that 
situation, and provide a caution to the user that what they're asking 
for isn't necessarily what they will be getting.
Thanks,
Eric

Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output

[Mika Westerberg]

On Tue, May 27, 2014 at 12:26:44PM -0700, eric.ernst@linux.intel.com wrote:
> From: Eric Ernst <eric.ernst@linux.intel.com>
> 
> For Baytrail, you should never set a GPIO set to direct_irq
> to output mode.  When direct_irq_en is set for a GPIO, it is
> tied directly to an APIC internally, and making the pad output
> does not make any sense. Assert a WARN() in the event this happens.

Can't we just clear that flag when the GPIO is requested? Or is this
something which can't be done at this point?

Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output

[eric ernst]

On 14-05-30 01:48 AM, Mika Westerberg wrote:
> On Tue, May 27, 2014 at 12:26:44PM -0700, eric.ernst@linux.intel.com wrote:
>> From: Eric Ernst <eric.ernst@linux.intel.com>
>>
>> For Baytrail, you should never set a GPIO set to direct_irq
>> to output mode.  When direct_irq_en is set for a GPIO, it is
>> tied directly to an APIC internally, and making the pad output
>> does not make any sense. Assert a WARN() in the event this happens.
> Can't we just clear that flag when the GPIO is requested? Or is this
> something which can't be done at this point?
The IRQ setting for that register (the IRQ wiring via direct_irq_en) 
should be set statically on boot, and should not be changed, let alone 
allowing for multiple changes during runtime.

Eric

Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output

[Mika Westerberg]

On Mon, Jun 02, 2014 at 11:35:16AM -0700, eric ernst wrote:
> On 14-05-30 01:48 AM, Mika Westerberg wrote:
> >On Tue, May 27, 2014 at 12:26:44PM -0700, eric.ernst@linux.intel.com wrote:
> >>From: Eric Ernst <eric.ernst@linux.intel.com>
> >>
> >>For Baytrail, you should never set a GPIO set to direct_irq
> >>to output mode.  When direct_irq_en is set for a GPIO, it is
> >>tied directly to an APIC internally, and making the pad output
> >>does not make any sense. Assert a WARN() in the event this happens.
> >Can't we just clear that flag when the GPIO is requested? Or is this
> >something which can't be done at this point?
> The IRQ setting for that register (the IRQ wiring via direct_irq_en) should
> be set statically on boot, and should not be changed, let alone allowing for
> multiple changes during runtime.

OK.

Please then follow the coding conventions documented in
Documentation/SubmittingPatches and also check that
scripts/checkpatch.pl doesn't complain too much.

Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output

[Mika Westerberg]

On Mon, Jun 02, 2014 at 10:13:22PM +0300, Mika Westerberg wrote:
> On Mon, Jun 02, 2014 at 11:35:16AM -0700, eric ernst wrote:
> > On 14-05-30 01:48 AM, Mika Westerberg wrote:
> > >On Tue, May 27, 2014 at 12:26:44PM -0700, eric.ernst@linux.intel.com wrote:
> > >>From: Eric Ernst <eric.ernst@linux.intel.com>
> > >>
> > >>For Baytrail, you should never set a GPIO set to direct_irq
> > >>to output mode.  When direct_irq_en is set for a GPIO, it is
> > >>tied directly to an APIC internally, and making the pad output
> > >>does not make any sense. Assert a WARN() in the event this happens.
> > >Can't we just clear that flag when the GPIO is requested? Or is this
> > >something which can't be done at this point?
> > The IRQ setting for that register (the IRQ wiring via direct_irq_en) should
> > be set statically on boot, and should not be changed, let alone allowing for
> > multiple changes during runtime.
> 
> OK.
> 
> Please then follow the coding conventions documented in
> Documentation/SubmittingPatches and also check that

Documentation/CodingStyle, that is.

> scripts/checkpatch.pl doesn't complain too much.