gpio-dwapb: reset mask register on probe
diff mbox

Message ID CACRpkdaknfTMb=1LVqNLTtV8Te+mjRdSZ=grk4NUGxKBt6gpCg@mail.gmail.com
State New
Headers show

Commit Message

Linus Walleij March 9, 2015, 1:59 p.m. UTC
On Tue, Mar 3, 2015 at 9:47 AM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:

> It's possible that boot-loader that worked on CPU before Linux kernel
> made some changes in GPIO controller registers. For example interrupts
> could be all masked.
>
> Current implementation of DW GPIO driver relies on default values in
> mask register.
>
> This is especially problematic in this DW GPIO driver because it sets 2
> pairs of methods: .irq_eable/.irq_disable and .irq_mask/.irq_unmask. In
> this case generic "irq_enable" function will use only
> .irq_enable call-back and mask register will be never modified so
> required interrupts will be finally unmasked.
>
> To troubleshoot described problem on driver probe we just need to make
> sure mask register is zeroed.
>
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Alexandre Courbot <gnurou@gmail.com>

Wait.

> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -370,6 +370,9 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>                 irq_create_mapping(gpio->domain, hwirq);
>
>         port->bgc.gc.to_irq = dwapb_gpio_to_irq;
> +
> +       /* Reset mask register */
> +       dwapb_write(gpio, GPIO_INTMASK, 0);

I don't get this. This looks like you just enable all interrupts. The
driver also contains this in .suspend():

/* Mask out interrupts */
dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);

If *anything* the probe should *mask* all interrupts so that the
.unmask() callback can enable them selectively.

The real problem I think is that struct irq_chip contains
mask()/unmask() callbacks that are not implemented
by this driver.

Can you please test the below (untested) patch instead:

From: Linus Walleij <linus.walleij@linaro.org>
Date: Mon, 9 Mar 2015 14:56:18 +0100
Subject: [PATCH] RFC: gpio: dwapb: handle mask/unmask properly

This implements the callbacks for masking/unmasking IRQs in the
special IRQ mask/unmask register of the DWAPB GPIO block.
Previously these mask bits were unhandled and relied on
boot-up defaults.

Reported-by: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-dwapb.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

     struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
@@ -302,6 +326,10 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
     struct irq_chip_type *ct;
     int err, i;

+    /* Mask out and disable all interrupts */
+    dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
+    dwapb_write(gpio, GPIO_INTEN, 0);
+
     gpio->domain = irq_domain_add_linear(node, ngpio,
                          &irq_generic_chip_ops, gpio);
     if (!gpio->domain)
@@ -334,6 +362,8 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
         ct->chip.irq_mask = irq_gc_mask_set_bit;
         ct->chip.irq_unmask = irq_gc_mask_clr_bit;
         ct->chip.irq_set_type = dwapb_irq_set_type;
+        ct->chip.irq_mask = dwapb_irq_mask;
+        ct->chip.irq_unmask = dwapb_irq_unmask;
         ct->chip.irq_enable = dwapb_irq_enable;
         ct->chip.irq_disable = dwapb_irq_disable;
         ct->chip.irq_request_resources = dwapb_irq_reqres;

Comments

Alexey Brodkin March 9, 2015, 8:56 p.m. UTC | #1
Hi Linus,

On Mon, 2015-03-09 at 14:59 +0100, Linus Walleij wrote:
> On Tue, Mar 3, 2015 at 9:47 AM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:

> > +++ b/drivers/gpio/gpio-dwapb.c
> > @@ -370,6 +370,9 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> >                 irq_create_mapping(gpio->domain, hwirq);
> >
> >         port->bgc.gc.to_irq = dwapb_gpio_to_irq;
> > +
> > +       /* Reset mask register */
> > +       dwapb_write(gpio, GPIO_INTMASK, 0);
> 
> I don't get this. This looks like you just enable all interrupts. The
> driver also contains this in .suspend():

DW APB GPIO has 2 separate registers related to interrupts:

 [1] Mask interrupt register
 [2] Enable interrupt register

So what I do in my patch I unmask all interrupts. But before at least
one interrupt is enabled output interrupt line will never get in active
state. And by default all interrupts are disabled (reset value = 0).

> /* Mask out interrupts */
> dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
> 
> If *anything* the probe should *mask* all interrupts so that the
> .unmask() callback can enable them selectively.

I'm going to agree with this statement, but this requires a bit more
significant change in driver. I just wanted to fix an issue I discovered
on my setup.

Interestingly what I observed in my testing that if both
enable()/disable() and mask()/unmask() are implemented in driver then
only enable()/disable() pair will be actually used.

Look at how generic irq_enable() function is implemented -
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/chip.c#n208

 --->8---
 void irq_enable(struct irq_desc *desc)
 {
	irq_state_clr_disabled(desc);
	if (desc->irq_data.chip->irq_enable)
		desc->irq_data.chip->irq_enable(&desc->irq_data);
	else
		desc->irq_data.chip->irq_unmask(&desc->irq_data);
	irq_state_clr_masked(desc);
 } 
 --->8---

> The real problem I think is that struct irq_chip contains
> mask()/unmask() callbacks that are not implemented
> by this driver.

I'd say that mask()/unmask() callbacks are implemented in this driver
already.

See
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-dwapb.c#n334
 --->8---
		ct->chip.irq_mask = irq_gc_mask_set_bit;
		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
		ct->chip.irq_set_type = dwapb_irq_set_type;
		ct->chip.irq_enable = dwapb_irq_enable;
		ct->chip.irq_disable = dwapb_irq_disable;
 --->8---

It actually uses generic implementation of mask set bit and clear bit:
irq_gc_mask_set_bit()/irq_gc_mask_clr_bit() that operate under
GPIO_INTMASK register. And I may confirm that these functions correctly
set/reset bits in mask register of GPIO controller.


> Can you please test the below (untested) patch instead:
> 
> From: Linus Walleij <linus.walleij@linaro.org>
> Date: Mon, 9 Mar 2015 14:56:18 +0100
> Subject: [PATCH] RFC: gpio: dwapb: handle mask/unmask properly
> 
> This implements the callbacks for masking/unmasking IRQs in the
> special IRQ mask/unmask register of the DWAPB GPIO block.
> Previously these mask bits were unhandled and relied on
> boot-up defaults.
> 
> Reported-by: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpio-dwapb.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 58faf04fce5d..1396f26bac5d 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -158,6 +158,30 @@ static void dwapb_irq_handler(u32 irq, struct
> irq_desc *desc)
>          chip->irq_eoi(irq_desc_get_irq_data(desc));
>  }
> 
> +static void dwapb_irq_mask(struct irq_data *d)
> +{
> +    struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
> +    struct dwapb_gpio *gpio = igc->private;
> +
> +    spin_lock_irqsave(&bgc->lock, flags);
> +    val = dwapb_read(gpio, GPIO_INTMASK);
> +    val |= BIT(d->hwirq);
> +    dwapb_write(gpio, GPIO_INTMASK, val);
> +    spin_unlock_irqrestore(&bgc->lock, flags);
> +}
> +
> +static void dwapb_irq_unmask(struct irq_data *d)
> +{
> +    struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
> +    struct dwapb_gpio *gpio = igc->private;
> +
> +    spin_lock_irqsave(&bgc->lock, flags);
> +    val = dwapb_read(gpio, GPIO_INTMASK);
> +    val &= ~BIT(d->hwirq);
> +    dwapb_write(gpio, GPIO_INTMASK, val);
> +    spin_unlock_irqrestore(&bgc->lock, flags);
> +}

Why would we need these custom functions if there're already
irq_gc_mask_set_bit()/irq_gc_mask_clr_bit() implemented in
kernel/irq/generic-chip.c

>  static void dwapb_irq_enable(struct irq_data *d)
>  {
>      struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
> @@ -302,6 +326,10 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>      struct irq_chip_type *ct;
>      int err, i;
> 
> +    /* Mask out and disable all interrupts */
> +    dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
> +    dwapb_write(gpio, GPIO_INTEN, 0);

This looks good to me - it's always a good idea to make sure defaults
are set as we expect.

>      gpio->domain = irq_domain_add_linear(node, ngpio,
>                           &irq_generic_chip_ops, gpio);
>      if (!gpio->domain)
> @@ -334,6 +362,8 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>          ct->chip.irq_mask = irq_gc_mask_set_bit;
>          ct->chip.irq_unmask = irq_gc_mask_clr_bit;
>          ct->chip.irq_set_type = dwapb_irq_set_type;
> +        ct->chip.irq_mask = dwapb_irq_mask;
> +        ct->chip.irq_unmask = dwapb_irq_unmask;

Looks like we set "ct->chip.irq_mask" and "ct->chip.irq_unmask" twice,
don't we?

-Alexey
--
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
Linus Walleij March 17, 2015, 4:54 p.m. UTC | #2
On Mon, Mar 9, 2015 at 9:56 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> On Mon, 2015-03-09 at 14:59 +0100, Linus Walleij wrote:
>> On Tue, Mar 3, 2015 at 9:47 AM, Alexey Brodkin
>> <Alexey.Brodkin@synopsys.com> wrote:

> Interestingly what I observed in my testing that if both
> enable()/disable() and mask()/unmask() are implemented in driver then
> only enable()/disable() pair will be actually used.
>
> Look at how generic irq_enable() function is implemented -
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/kernel/irq/chip.c#n208
>
>  --->8---
>  void irq_enable(struct irq_desc *desc)
>  {
>         irq_state_clr_disabled(desc);
>         if (desc->irq_data.chip->irq_enable)
>                 desc->irq_data.chip->irq_enable(&desc->irq_data);
>         else
>                 desc->irq_data.chip->irq_unmask(&desc->irq_data);
>         irq_state_clr_masked(desc);
>  }
>  --->8---
>
>> The real problem I think is that struct irq_chip contains
>> mask()/unmask() callbacks that are not implemented
>> by this driver.
>
> I'd say that mask()/unmask() callbacks are implemented in this driver
> already.
>
> See
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-dwapb.c#n334
>  --->8---
>                 ct->chip.irq_mask = irq_gc_mask_set_bit;
>                 ct->chip.irq_unmask = irq_gc_mask_clr_bit;
>                 ct->chip.irq_set_type = dwapb_irq_set_type;
>                 ct->chip.irq_enable = dwapb_irq_enable;
>                 ct->chip.irq_disable = dwapb_irq_disable;
>  --->8---
>
> It actually uses generic implementation of mask set bit and clear bit:
> irq_gc_mask_set_bit()/irq_gc_mask_clr_bit() that operate under
> GPIO_INTMASK register. And I may confirm that these functions correctly
> set/reset bits in mask register of GPIO controller.

Grrr how typical, I got it all wrong and I'm doing stupid things :(

So you mean these generic mask/unmask callbacks sets the bits
correctly and then there is no problem.

>> +static void dwapb_irq_mask(struct irq_data *d)
>> +static void dwapb_irq_unmask(struct irq_data *d)
>
> Why would we need these custom functions if there're already
> irq_gc_mask_set_bit()/irq_gc_mask_clr_bit() implemented in
> kernel/irq/generic-chip.c

You're right...

>>  static void dwapb_irq_enable(struct irq_data *d)
>>  {
>>      struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
>> @@ -302,6 +326,10 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>>      struct irq_chip_type *ct;
>>      int err, i;
>>
>> +    /* Mask out and disable all interrupts */
>> +    dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
>> +    dwapb_write(gpio, GPIO_INTEN, 0);
>
> This looks good to me - it's always a good idea to make sure defaults
> are set as we expect.

So should I cook a patch doing just these two lines?

But the initial patch unmasking all IRQs then? Is that even needed?

>>      gpio->domain = irq_domain_add_linear(node, ngpio,
>>                           &irq_generic_chip_ops, gpio);
>>      if (!gpio->domain)
>> @@ -334,6 +362,8 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>>          ct->chip.irq_mask = irq_gc_mask_set_bit;
>>          ct->chip.irq_unmask = irq_gc_mask_clr_bit;
>>          ct->chip.irq_set_type = dwapb_irq_set_type;
>> +        ct->chip.irq_mask = dwapb_irq_mask;
>> +        ct->chip.irq_unmask = dwapb_irq_unmask;
>
> Looks like we set "ct->chip.irq_mask" and "ct->chip.irq_unmask" twice,
> don't we?

Yep, my bad.

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
Alexey Brodkin March 19, 2015, 8:44 a.m. UTC | #3
Hi Linus,

On Tue, 2015-03-17 at 17:54 +0100, Linus Walleij wrote:
> >> +    /* Mask out and disable all interrupts */
> >> +    dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
> >> +    dwapb_write(gpio, GPIO_INTEN, 0);
> >
> > This looks good to me - it's always a good idea to make sure defaults
> > are set as we expect.
> 
> So should I cook a patch doing just these two lines?
> 
> But the initial patch unmasking all IRQs then? Is that even needed?

Oops, my bad here :)

I intentionally wrote 0 to MASK register to return it to default state
(if pre-bootloader messes it). As I explained if we really mask all
interrupts (together with disabling them all via INTEN) then interrupts
will never happen.

If we look at DW APB GPIO databook it says:
--->8---
Whenever a 1 is written to a bit of this register [GPIO_INTEN], it
configures the corresponding bit on Port A to become an interrupt;
otherwise, Port A operates as a normal GPIO signal.
--->8---

While for GPIO_INTMASK it says:
--->8---
Controls whether an interrupt on Port A can create an interrupt for the
interrupt controller by not masking it. By default, all interrupts bits
are unmasked.
--->8---

So IMHO we need to update "gpio-dwapb" driver in the following manner:

 [1] In dwapb_configure_irqs() in accordance to "snps,nr-gpios" in the
first bank set a value in GPIO_INTEN. This way we turn N pins in that
port/bank in "interrupt" mode from their default gpio mode.

 [2] Don't expose dwapb_irq_enable()/dwapb_irq_disable() through
ct->chip.irq_enable/ct->chip.irq_disable. Because we don't want to
toggle modes of pins, right?

 [3] Use ct->chip.irq_mask/ct->chip.irq_unmask for purpose of real
enabling/disabling interrupts.

If I'm not missing something that would be implementation that matches
real device specification.

If we do these changes then indeed we'll want to make sure GPIO_INTMASK
is initialized with all interrupts masked out:
--->8---
dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
--->8---

Let me know if my proposal makes sense and then I'll send patch that
implements it.

-Alexey
--
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
Linus Walleij March 26, 2015, 9:29 a.m. UTC | #4
On Thu, Mar 19, 2015 at 9:44 AM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:

> So IMHO we need to update "gpio-dwapb" driver in the following manner:
>
>  [1] In dwapb_configure_irqs() in accordance to "snps,nr-gpios" in the
> first bank set a value in GPIO_INTEN. This way we turn N pins in that
> port/bank in "interrupt" mode from their default gpio mode.
>
>  [2] Don't expose dwapb_irq_enable()/dwapb_irq_disable() through
> ct->chip.irq_enable/ct->chip.irq_disable. Because we don't want to
> toggle modes of pins, right?
>
>  [3] Use ct->chip.irq_mask/ct->chip.irq_unmask for purpose of real
> enabling/disabling interrupts.
>
> If I'm not missing something that would be implementation that matches
> real device specification.
>
> If we do these changes then indeed we'll want to make sure GPIO_INTMASK
> is initialized with all interrupts masked out:
> --->8---
> dwapb_write(gpio, GPIO_INTMASK, 0xffffffff);
> --->8---
>
> Let me know if my proposal makes sense and then I'll send patch that
> implements it.

Sure as long as you can test it and it works, I'm fine with this.

I might be looking at migrating this quite popular driver to
GPIOLIB_IRQCHIP so would be happy if you can help out
testing it if I do this later.

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

Patch
diff mbox

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 58faf04fce5d..1396f26bac5d 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -158,6 +158,30 @@  static void dwapb_irq_handler(u32 irq, struct
irq_desc *desc)
         chip->irq_eoi(irq_desc_get_irq_data(desc));
 }

+static void dwapb_irq_mask(struct irq_data *d)
+{
+    struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
+    struct dwapb_gpio *gpio = igc->private;
+
+    spin_lock_irqsave(&bgc->lock, flags);
+    val = dwapb_read(gpio, GPIO_INTMASK);
+    val |= BIT(d->hwirq);
+    dwapb_write(gpio, GPIO_INTMASK, val);
+    spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static void dwapb_irq_unmask(struct irq_data *d)
+{
+    struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
+    struct dwapb_gpio *gpio = igc->private;
+
+    spin_lock_irqsave(&bgc->lock, flags);
+    val = dwapb_read(gpio, GPIO_INTMASK);
+    val &= ~BIT(d->hwirq);
+    dwapb_write(gpio, GPIO_INTMASK, val);
+    spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
 static void dwapb_irq_enable(struct irq_data *d)
 {