| Message ID | CACRpkdaknfTMb=1LVqNLTtV8Te+mjRdSZ=grk4NUGxKBt6gpCg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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) {