linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output
@ 2014-05-27 19:26 eric.ernst
  2014-05-29 13:44 ` Linus Walleij
  2014-05-30  8:48 ` Mika Westerberg
  0 siblings, 2 replies; 7+ messages in thread
From: eric.ernst @ 2014-05-27 19:26 UTC (permalink / raw)
  To: linus.walleij, linux-kernel, mark.gross; +Cc: 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output
  2014-05-27 19:26 [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output eric.ernst
@ 2014-05-29 13:44 ` Linus Walleij
  2014-05-29 21:57   ` eric ernst
  2014-05-30  8:48 ` Mika Westerberg
  1 sibling, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2014-05-29 13:44 UTC (permalink / raw)
  To: eric.ernst, Mika Westerberg, Mathias Nyman, Andy Shevchenko
  Cc: linux-kernel, Mark Gross, Holmberg, Hans

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output
  2014-05-29 13:44 ` Linus Walleij
@ 2014-05-29 21:57   ` eric ernst
  0 siblings, 0 replies; 7+ messages in thread
From: eric ernst @ 2014-05-29 21:57 UTC (permalink / raw)
  To: Linus Walleij, Mika Westerberg, Mathias Nyman, Andy Shevchenko
  Cc: linux-kernel, Mark Gross, Holmberg, Hans

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output
  2014-05-27 19:26 [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output eric.ernst
  2014-05-29 13:44 ` Linus Walleij
@ 2014-05-30  8:48 ` Mika Westerberg
  2014-06-02 18:35   ` eric ernst
  1 sibling, 1 reply; 7+ messages in thread
From: Mika Westerberg @ 2014-05-30  8:48 UTC (permalink / raw)
  To: eric.ernst; +Cc: linus.walleij, linux-kernel, mark.gross

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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output
  2014-05-30  8:48 ` Mika Westerberg
@ 2014-06-02 18:35   ` eric ernst
  2014-06-02 19:13     ` Mika Westerberg
  0 siblings, 1 reply; 7+ messages in thread
From: eric ernst @ 2014-06-02 18:35 UTC (permalink / raw)
  To: Mika Westerberg; +Cc: linus.walleij, linux-kernel, mark.gross

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output
  2014-06-02 18:35   ` eric ernst
@ 2014-06-02 19:13     ` Mika Westerberg
  2014-06-02 19:14       ` Mika Westerberg
  0 siblings, 1 reply; 7+ messages in thread
From: Mika Westerberg @ 2014-06-02 19:13 UTC (permalink / raw)
  To: eric ernst; +Cc: linus.walleij, linux-kernel, mark.gross

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output
  2014-06-02 19:13     ` Mika Westerberg
@ 2014-06-02 19:14       ` Mika Westerberg
  0 siblings, 0 replies; 7+ messages in thread
From: Mika Westerberg @ 2014-06-02 19:14 UTC (permalink / raw)
  To: eric ernst; +Cc: linus.walleij, linux-kernel, mark.gross

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-06-02 19:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-27 19:26 [PATCH 1/1] PINCTRL: Warn if direct IRQ GPIO set to output eric.ernst
2014-05-29 13:44 ` Linus Walleij
2014-05-29 21:57   ` eric ernst
2014-05-30  8:48 ` Mika Westerberg
2014-06-02 18:35   ` eric ernst
2014-06-02 19:13     ` Mika Westerberg
2014-06-02 19:14       ` Mika Westerberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).