* [PATCH] gpio-dwapb: reset mask register on probe @ 2015-03-03 8:47 Alexey Brodkin 2015-03-09 13:59 ` Linus Walleij 0 siblings, 1 reply; 6+ messages in thread From: Alexey Brodkin @ 2015-03-03 8:47 UTC (permalink / raw) To: linux-gpio Cc: linux-kernel, Alexey Brodkin, Vineet Gupta, Linus Walleij, Alexandre Courbot 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> --- drivers/gpio/gpio-dwapb.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 58faf04..be75860 100644 --- a/drivers/gpio/gpio-dwapb.c +++ 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); } static void dwapb_irq_teardown(struct dwapb_gpio *gpio) -- 2.1.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio-dwapb: reset mask register on probe 2015-03-03 8:47 [PATCH] gpio-dwapb: reset mask register on probe Alexey Brodkin @ 2015-03-09 13:59 ` Linus Walleij 2015-03-09 20:56 ` Alexey Brodkin 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2015-03-09 13:59 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, Vineet Gupta, Alexandre Courbot 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(+) 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) { 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; -- 1.9.3 Yours, Linus Walleij ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio-dwapb: reset mask register on probe 2015-03-09 13:59 ` Linus Walleij @ 2015-03-09 20:56 ` Alexey Brodkin 2015-03-17 16:54 ` Linus Walleij 0 siblings, 1 reply; 6+ messages in thread From: Alexey Brodkin @ 2015-03-09 20:56 UTC (permalink / raw) To: linus.walleij@linaro.org Cc: linux-kernel@vger.kernel.org, Vineet.Gupta1@synopsys.com, linux-gpio@vger.kernel.org, gnurou@gmail.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio-dwapb: reset mask register on probe 2015-03-09 20:56 ` Alexey Brodkin @ 2015-03-17 16:54 ` Linus Walleij 2015-03-19 8:44 ` Alexey Brodkin 0 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2015-03-17 16:54 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-kernel@vger.kernel.org, Vineet.Gupta1@synopsys.com, linux-gpio@vger.kernel.org, gnurou@gmail.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio-dwapb: reset mask register on probe 2015-03-17 16:54 ` Linus Walleij @ 2015-03-19 8:44 ` Alexey Brodkin 2015-03-26 9:29 ` Linus Walleij 0 siblings, 1 reply; 6+ messages in thread From: Alexey Brodkin @ 2015-03-19 8:44 UTC (permalink / raw) To: linus.walleij@linaro.org Cc: linux-kernel@vger.kernel.org, Vineet.Gupta1@synopsys.com, linux-gpio@vger.kernel.org, gnurou@gmail.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gpio-dwapb: reset mask register on probe 2015-03-19 8:44 ` Alexey Brodkin @ 2015-03-26 9:29 ` Linus Walleij 0 siblings, 0 replies; 6+ messages in thread From: Linus Walleij @ 2015-03-26 9:29 UTC (permalink / raw) To: Alexey Brodkin Cc: linux-kernel@vger.kernel.org, Vineet.Gupta1@synopsys.com, linux-gpio@vger.kernel.org, gnurou@gmail.com 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-03-26 9:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-03 8:47 [PATCH] gpio-dwapb: reset mask register on probe Alexey Brodkin 2015-03-09 13:59 ` Linus Walleij 2015-03-09 20:56 ` Alexey Brodkin 2015-03-17 16:54 ` Linus Walleij 2015-03-19 8:44 ` Alexey Brodkin 2015-03-26 9:29 ` Linus Walleij
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).