All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/1] pinctrl: intel: Set pin direction properly
@ 2017-01-02 12:07 Andy Shevchenko
  2017-01-09 14:29 ` Jarkko Nikula
  2017-01-11 12:50 ` Linus Walleij
  0 siblings, 2 replies; 7+ messages in thread
From: Andy Shevchenko @ 2017-01-02 12:07 UTC (permalink / raw)
  To: Linus Walleij, linux-gpio, Mika Westerberg, Jarkko Nikula; +Cc: Andy Shevchenko

There are two bits in the PADCFG0 register to configure direction, one per
TX/RX buffers.

For now we wrongly assume that the GPIO is always requested before it is being
used, which is not true when the GPIO is used through irqchip. In this case the
GPIO is never requested and we never enable RX buffer for it.

Fix this by setting both bits accordingly.

Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 1e139672f1af..6df35dcb29ae 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -353,6 +353,21 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned function,
 	return 0;
 }
 
+static void __intel_gpio_set_direction(void __iomem *padcfg0, bool input)
+{
+	u32 value;
+
+	value = readl(padcfg0);
+	if (input) {
+		value &= ~PADCFG0_GPIORXDIS;
+		value |= PADCFG0_GPIOTXDIS;
+	} else {
+		value &= ~PADCFG0_GPIOTXDIS;
+		value |= PADCFG0_GPIORXDIS;
+	}
+	writel(value, padcfg0);
+}
+
 static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 				     struct pinctrl_gpio_range *range,
 				     unsigned pin)
@@ -375,11 +390,11 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	/* 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);
 
+	/* Disable TX buffer and enable RX (this will be input) */
+	__intel_gpio_set_direction(padcfg0, true);
+
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	return 0;
@@ -392,18 +407,11 @@ static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	void __iomem *padcfg0;
 	unsigned long flags;
-	u32 value;
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
-
-	value = readl(padcfg0);
-	if (input)
-		value |= PADCFG0_GPIOTXDIS;
-	else
-		value &= ~PADCFG0_GPIOTXDIS;
-	writel(value, padcfg0);
+	__intel_gpio_set_direction(padcfg0, input);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
-- 
2.11.0


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

* Re: [PATCH v1 1/1] pinctrl: intel: Set pin direction properly
  2017-01-02 12:07 [PATCH v1 1/1] pinctrl: intel: Set pin direction properly Andy Shevchenko
@ 2017-01-09 14:29 ` Jarkko Nikula
  2017-01-09 14:45   ` Andy Shevchenko
  2017-01-11 12:50 ` Linus Walleij
  1 sibling, 1 reply; 7+ messages in thread
From: Jarkko Nikula @ 2017-01-09 14:29 UTC (permalink / raw)
  To: Andy Shevchenko, Linus Walleij, linux-gpio, Mika Westerberg

On 01/02/2017 02:07 PM, Andy Shevchenko wrote:
> There are two bits in the PADCFG0 register to configure direction, one per
> TX/RX buffers.
>
> For now we wrongly assume that the GPIO is always requested before it is being
> used, which is not true when the GPIO is used through irqchip. In this case the
> GPIO is never requested and we never enable RX buffer for it.
>
> Fix this by setting both bits accordingly.
>
> Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
>
...
> @@ -375,11 +390,11 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
> @@ -392,18 +407,11 @@ static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,

I'm testing this on top of v4.10.0-rc3 and I don't see changes in 
PADCFG0 after this patch. I guess reason is that the code doesn't go 
through above functions for the pin that is used through irqchip but 
through intel_gpio_irq_type().

Am I missing some another patch or should your patch add 
__intel_gpio_set_direction() also there?

-- 
Jarkko

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

* Re: [PATCH v1 1/1] pinctrl: intel: Set pin direction properly
  2017-01-09 14:29 ` Jarkko Nikula
@ 2017-01-09 14:45   ` Andy Shevchenko
  2017-01-09 15:12     ` Jarkko Nikula
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Shevchenko @ 2017-01-09 14:45 UTC (permalink / raw)
  To: Jarkko Nikula, Linus Walleij, linux-gpio, Mika Westerberg

On Mon, 2017-01-09 at 16:29 +0200, Jarkko Nikula wrote:
> On 01/02/2017 02:07 PM, Andy Shevchenko wrote:
> > There are two bits in the PADCFG0 register to configure direction,
> > one per
> > TX/RX buffers.
> > 
> > For now we wrongly assume that the GPIO is always requested before
> > it is being
> > used, which is not true when the GPIO is used through irqchip. In
> > this case the
> > GPIO is never requested and we never enable RX buffer for it.
> > 
> > Fix this by setting both bits accordingly.
> > 
> > Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > 


> > @@ -392,18 +407,11 @@ static int intel_gpio_set_direction(struct
> > pinctrl_dev *pctldev,
> 
> I'm testing this on top of v4.10.0-rc3 and I don't see changes in 
> PADCFG0 after this patch. I guess reason is that the code doesn't go 
> through above functions for the pin that is used through irqchip but 
> through intel_gpio_irq_type().
> 
> Am I missing some another patch or should your patch add 
> __intel_gpio_set_direction() also there?

The problem you reported about apparently discovers two places to be
fixed. This is part 1. Part 2 will be send with GPIO ACPI clean up / bug
fix series later.

The rest of patches is available on my public tree:
https://bitbucket.org/andy-shev/linux/branch/topic%2Fuart%2frpm

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

* Re: [PATCH v1 1/1] pinctrl: intel: Set pin direction properly
  2017-01-09 14:45   ` Andy Shevchenko
@ 2017-01-09 15:12     ` Jarkko Nikula
  2017-01-09 15:41       ` Andy Shevchenko
  0 siblings, 1 reply; 7+ messages in thread
From: Jarkko Nikula @ 2017-01-09 15:12 UTC (permalink / raw)
  To: Andy Shevchenko, Linus Walleij, linux-gpio, Mika Westerberg

On 01/09/2017 04:45 PM, Andy Shevchenko wrote:
> On Mon, 2017-01-09 at 16:29 +0200, Jarkko Nikula wrote:
>> On 01/02/2017 02:07 PM, Andy Shevchenko wrote:
>>> There are two bits in the PADCFG0 register to configure direction,
>>> one per
>>> TX/RX buffers.
>>>
>>> For now we wrongly assume that the GPIO is always requested before
>>> it is being
>>> used, which is not true when the GPIO is used through irqchip. In
>>> this case the
>>> GPIO is never requested and we never enable RX buffer for it.
>>>
>>> Fix this by setting both bits accordingly.
>>>
>>> Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>>>
>
>
>>> @@ -392,18 +407,11 @@ static int intel_gpio_set_direction(struct
>>> pinctrl_dev *pctldev,
>>
>> I'm testing this on top of v4.10.0-rc3 and I don't see changes in
>> PADCFG0 after this patch. I guess reason is that the code doesn't go
>> through above functions for the pin that is used through irqchip but
>> through intel_gpio_irq_type().
>>
>> Am I missing some another patch or should your patch add
>> __intel_gpio_set_direction() also there?
>
> The problem you reported about apparently discovers two places to be
> fixed. This is part 1. Part 2 will be send with GPIO ACPI clean up / bug
> fix series later.
>
Should the commit log be refined a bit as this patch doesn't fix the 
issue but prepares for the fix by adding the RX pad control in 
intel_gpio_set_direction()?

-- 
Jarkko

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

* Re: [PATCH v1 1/1] pinctrl: intel: Set pin direction properly
  2017-01-09 15:12     ` Jarkko Nikula
@ 2017-01-09 15:41       ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2017-01-09 15:41 UTC (permalink / raw)
  To: Jarkko Nikula, Linus Walleij, linux-gpio, Mika Westerberg

On Mon, 2017-01-09 at 17:12 +0200, Jarkko Nikula wrote:
> On 01/09/2017 04:45 PM, Andy Shevchenko wrote:
> > On Mon, 2017-01-09 at 16:29 +0200, Jarkko Nikula wrote:
> > > On 01/02/2017 02:07 PM, Andy Shevchenko wrote:
> > > > There are two bits in the PADCFG0 register to configure
> > > > direction,
> > > > one per
> > > > TX/RX buffers.
> > > > 
> > > > For now we wrongly assume that the GPIO is always requested
> > > > before
> > > > it is being
> > > > used, which is not true when the GPIO is used through irqchip.
> > > > In
> > > > this case the
> > > > GPIO is never requested and we never enable RX buffer for it.
> > > > 
> > > > Fix this by setting both bits accordingly.
> > > > 
> > > > Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > > 
> > 
> > 
> > > > @@ -392,18 +407,11 @@ static int intel_gpio_set_direction(struct
> > > > pinctrl_dev *pctldev,
> > > 
> > > I'm testing this on top of v4.10.0-rc3 and I don't see changes in
> > > PADCFG0 after this patch. I guess reason is that the code doesn't
> > > go
> > > through above functions for the pin that is used through irqchip
> > > but
> > > through intel_gpio_irq_type().
> > > 
> > > Am I missing some another patch or should your patch add
> > > __intel_gpio_set_direction() also there?
> > 
> > The problem you reported about apparently discovers two places to be
> > fixed. This is part 1. Part 2 will be send with GPIO ACPI clean up /
> > bug
> > fix series later.
> > 
> 
> Should the commit log be refined a bit as this patch doesn't fix the 
> issue but prepares for the fix by adding the RX pad control in 
> intel_gpio_set_direction()?

I don't think we need this. Basically I split your Reported-by to the
two patches (okay, I need to check if I put it to another one). 

The problems they solve are kinda independent, but both of them are
parts of the issue you faced.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

* Re: [PATCH v1 1/1] pinctrl: intel: Set pin direction properly
  2017-01-02 12:07 [PATCH v1 1/1] pinctrl: intel: Set pin direction properly Andy Shevchenko
  2017-01-09 14:29 ` Jarkko Nikula
@ 2017-01-11 12:50 ` Linus Walleij
  2017-01-11 14:21   ` Andy Shevchenko
  1 sibling, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2017-01-11 12:50 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-gpio, Mika Westerberg, Jarkko Nikula

On Mon, Jan 2, 2017 at 1:07 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> There are two bits in the PADCFG0 register to configure direction, one per
> TX/RX buffers.
>
> For now we wrongly assume that the GPIO is always requested before it is being
> used, which is not true when the GPIO is used through irqchip. In this case the
> GPIO is never requested and we never enable RX buffer for it.
>
> Fix this by setting both bits accordingly.
>
> Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I applied this patch for fixes on top of Mika's pad offset
fix.

Hope this is right... There is some noise in the thread but
AFAICT there are more fixes coming.

Yours,
Linus Walleij

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

* Re: [PATCH v1 1/1] pinctrl: intel: Set pin direction properly
  2017-01-11 12:50 ` Linus Walleij
@ 2017-01-11 14:21   ` Andy Shevchenko
  0 siblings, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2017-01-11 14:21 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-gpio, Mika Westerberg, Jarkko Nikula

On Wed, 2017-01-11 at 13:50 +0100, Linus Walleij wrote:
> On Mon, Jan 2, 2017 at 1:07 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > There are two bits in the PADCFG0 register to configure direction,
> > one per
> > TX/RX buffers.
> > 
> > For now we wrongly assume that the GPIO is always requested before
> > it is being
> > used, which is not true when the GPIO is used through irqchip. In
> > this case the
> > GPIO is never requested and we never enable RX buffer for it.
> > 
> > Fix this by setting both bits accordingly.
> > 
> > Reported-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I applied this patch for fixes on top of Mika's pad offset
> fix.
> 
> Hope this is right... There is some noise in the thread but
> AFAICT there are more fixes coming.

Yes, it's self-sufficient and needed independently of the rest.
Thanks!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

end of thread, other threads:[~2017-01-11 14:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-02 12:07 [PATCH v1 1/1] pinctrl: intel: Set pin direction properly Andy Shevchenko
2017-01-09 14:29 ` Jarkko Nikula
2017-01-09 14:45   ` Andy Shevchenko
2017-01-09 15:12     ` Jarkko Nikula
2017-01-09 15:41       ` Andy Shevchenko
2017-01-11 12:50 ` Linus Walleij
2017-01-11 14:21   ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.