| Message ID | 20170102120722.178343-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
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?
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
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()?
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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);
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(-)