PINCTRL: Warn if direct IRQ GPIO set to output

PATCH Superseded [email protected] 3 participants 7 messages May 27, 2014 – Jun 2, 2014 Pin control

lore

Share & tools

lore

[email protected]

author

2014-05-27 19:28 UTC #0 lore raw

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

From: Eric Ernst <[email protected]>

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 <[email protected]>

---
 drivers/pinctrl/pinctrl-baytrail.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/pinctrl/pinctrl-baytrail.c +12 -4

index e59983423991..677f79a9efca 100644
@@ -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);

Signature · 1 line

1.7.9.5

Linus Walleij

2014-05-29 13:45 UTC #1 lore raw

Quoted 8 lines · On Tue, May 27, 2014 at 9:26 PM, <[email protected]> wrote:

From: Eric Ernst <[email protected]>
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 <[email protected]>

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

eric ernst

author

2014-05-29 22:00 UTC #2 lore raw

Quoted 10 lines · Linus Walleij

Quoted 8 lines · On Tue, May 27, 2014 at 9:26 PM, <[email protected]> wrote:
From: Eric Ernst <[email protected]>
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 <[email protected]>

Quoted 8 lines

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

Mika Westerberg

2014-05-30 08:48 UTC #3 lore raw

In reply to [email protected]

Quoted 6 lines · On Tue, May 27, 2014 at 12:26:44PM -0700, [email protected] wrote:

From: Eric Ernst <[email protected]>
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?

eric ernst

author

2014-06-02 18:37 UTC #4 lore raw

Quoted 9 lines · Mika Westerberg

Quoted 6 lines · On Tue, May 27, 2014 at 12:26:44PM -0700, [email protected] wrote:
From: Eric Ernst <[email protected]>
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

Mika Westerberg

2014-06-02 19:13 UTC #5 lore raw

Quoted 8 lines · eric ernst

Quoted 7 lines · Mika Westerberg
Quoted 6 lines · On Tue, May 27, 2014 at 12:26:44PM -0700, [email protected] wrote:
From: Eric Ernst <[email protected]>
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.

Quoted 5 lines

Quoted 2 lines
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.

Mika Westerberg

2014-06-02 19:14 UTC #6 lore raw

Quoted 11 lines · Mika Westerberg

Quoted 10 lines · eric ernst
Quoted 9 lines · Mika Westerberg
Quoted 6 lines · On Tue, May 27, 2014 at 12:26:44PM -0700, [email protected] wrote:
From: Eric Ernst <[email protected]>
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?

Quoted 8 lines

Quoted 3 lines
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.

Quoted 1 line

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