| Message ID | 20140122221237.GA30763@localhost |
|---|---|
| State | New |
| Headers | show |
On Wed, Jan 22, 2014 at 07:12:38PM -0300, Ezequiel Garcia wrote: > On Wed, Jan 22, 2014 at 01:52:13PM -0700, Jason Gunthorpe wrote: > > > Clearing BRIDGE_CAUSE will only clear all currently pending upstream > > > IRQs, of course. If WDT IRQ will be re-raised right after that in > > > BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no > > > clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE. > > > > Which is why it makes no sense to clear it one time at kernel start. > > > > So, it seems we need to handle irq_startup(), as you suggested. > I've just tested the attached patch, and it's working fine: the driver's > probe() fully stops the watchdog, and then request_irq() acks and > pending interrupts, through the added irq_startup(). > > How does it look? Looks sane to me. I looked some more and there are other drivers (eg irq-metag-ext) that take this same approach. Sebastian: I looked at the irq-orion driver a bit more and noticed this: ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name, handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE); ^^^^^^^^^^^^^^^^^^^^^ Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How does this work at all? :) Jason
On Wed, Jan 22, 2014 at 03:31:17PM -0700, Jason Gunthorpe wrote: > On Wed, Jan 22, 2014 at 07:12:38PM -0300, Ezequiel Garcia wrote: > > On Wed, Jan 22, 2014 at 01:52:13PM -0700, Jason Gunthorpe wrote: > > > > Clearing BRIDGE_CAUSE will only clear all currently pending upstream > > > > IRQs, of course. If WDT IRQ will be re-raised right after that in > > > > BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no > > > > clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE. > > > > > > Which is why it makes no sense to clear it one time at kernel start. > > > > > > > So, it seems we need to handle irq_startup(), as you suggested. > > I've just tested the attached patch, and it's working fine: the driver's > > probe() fully stops the watchdog, and then request_irq() acks and > > pending interrupts, through the added irq_startup(). > > > > How does it look? > > Looks sane to me. > > I looked some more and there are other drivers (eg irq-metag-ext) that > take this same approach. > Yup, I took that one as a starting point. [..] > I looked at the irq-orion driver a bit more and noticed this: > > ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name, > handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE); > ^^^^^^^^^^^^^^^^^^^^^ > Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How > does this work at all? :) > I'm not familiar with the differences between handle_level_irq and handle_edge_irq, but -AFAICS- both seem to ack the IRQ. In fact handle_level_irq(), masks and acks the IRQ as the first thing.
On 01/22/2014 11:31 PM, Jason Gunthorpe wrote: > On Wed, Jan 22, 2014 at 07:12:38PM -0300, Ezequiel Garcia wrote: >> On Wed, Jan 22, 2014 at 01:52:13PM -0700, Jason Gunthorpe wrote: >>>> Clearing BRIDGE_CAUSE will only clear all currently pending upstream >>>> IRQs, of course. If WDT IRQ will be re-raised right after that in >>>> BRIDGE_CAUSE depends on the actual HW implementation, i.e. we do no >>>> clear the causing IRQ itself but just what it raised in BRIDGE_CAUSE. >>> >>> Which is why it makes no sense to clear it one time at kernel start. >>> >> >> So, it seems we need to handle irq_startup(), as you suggested. >> I've just tested the attached patch, and it's working fine: the driver's >> probe() fully stops the watchdog, and then request_irq() acks and >> pending interrupts, through the added irq_startup(). >> >> How does it look? > > Looks sane to me. > > I looked some more and there are other drivers (eg irq-metag-ext) that > take this same approach. > > Sebastian: > I looked at the irq-orion driver a bit more and noticed this: > > ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name, > handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE); > ^^^^^^^^^^^^^^^^^^^^^ > Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How > does this work at all? :) I can tell you that it comes from arch/arm/plat-orion/irq.c and I blindly copied it. I never really checked the differences in handling level/edge irqs. Besides, if it wasn't working, we wouldn't get far in booting the kernel without timer irqs. From a HW point-of-view, I'd say the difference is that an edge-triggered irq samples level transitions from e.g. low to high. It also remains asserted if you clear the actual cause of the interrupt and is only asserted again on the next low-to-high transition. A level-triggered irq will be asserted as long as the cause of the irq is asserted. If you clear the cause, you'll also clear that bit in the upstream controller. *BUT*, I will double-check how Linux deals with level/edge irqs and if Orion SoCs have edge or level triggered cause registers. That should reveal, if it is more sane to use handle_edge_irq here and possibly in the main interrupt controller, too. Sebastian
On Thu, Jan 23, 2014 at 01:03:26AM +0100, Sebastian Hesselbarth wrote: > >Sebastian: > >I looked at the irq-orion driver a bit more and noticed this: > > > > ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name, > > handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE); > > ^^^^^^^^^^^^^^^^^^^^^ > >Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How > >does this work at all? :) > > I can tell you that it comes from arch/arm/plat-orion/irq.c and I > blindly copied it. I never really checked the differences in handling > level/edge irqs. Besides, if it wasn't working, we wouldn't get far > in booting the kernel without timer irqs. Ezequiel found the ack call I missed, so it makes sense it works. I think the difference in routines only starts to matter when you can get another incoming edge IRQ while already handling one (due to SMP? threaded interrupts? RealTime? not sure) > It also remains asserted if you clear the actual cause of the interrupt > and is only asserted again on the next low-to-high transition. Which is why Ezequiel's patch is the right approach: we need to clear the interrupt latched in the cause register after the watchdog driver disable but before enabling/unmasking the interrupt. Remember, the BRIDGE_MASK register has no effect on the BRIDGE_CAUSE, it only effects which bits propogate to the main cause register. > *BUT*, I will double-check how Linux deals with level/edge irqs and if > Orion SoCs have edge or level triggered cause registers. That should > reveal, if it is more sane to use handle_edge_irq here and possibly in > the main interrupt controller, too. There is a mixture. Any cause bit documented to be clearable is edge triggered, all others are level. On Kirkwood this means all of the main interrupt controller bits are level and all the bridge bits are edge. Which means edge is definitely correct for the bridge handler, and level correct for the main handler. Jason
On 01/23/2014 01:19 AM, Jason Gunthorpe wrote: > On Thu, Jan 23, 2014 at 01:03:26AM +0100, Sebastian Hesselbarth wrote: >>> Sebastian: >>> I looked at the irq-orion driver a bit more and noticed this: >>> >>> ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name, >>> handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE); >>> ^^^^^^^^^^^^^^^^^^^^^ >>> Shouldn't it be handle_edge_irq? Otherwise who is calling irq_ack? How >>> does this work at all? :) >> >> I can tell you that it comes from arch/arm/plat-orion/irq.c and I >> blindly copied it. I never really checked the differences in handling >> level/edge irqs. Besides, if it wasn't working, we wouldn't get far >> in booting the kernel without timer irqs. > > Ezequiel found the ack call I missed, so it makes sense it works. > > I think the difference in routines only starts to matter when you can > get another incoming edge IRQ while already handling one (due to SMP? > threaded interrupts? RealTime? not sure) > >> It also remains asserted if you clear the actual cause of the interrupt >> and is only asserted again on the next low-to-high transition. > > Which is why Ezequiel's patch is the right approach: we need to clear > the interrupt latched in the cause register after the watchdog driver > disable but before enabling/unmasking the interrupt. > > Remember, the BRIDGE_MASK register has no effect on the BRIDGE_CAUSE, > it only effects which bits propogate to the main cause register. Yeah, I know. But you don't get new ones if mask them. At least for edge triggered irqs, you can also clear them without clearing the cause of the interrupt. Nevertheless, I think we agree here. >> *BUT*, I will double-check how Linux deals with level/edge irqs and if >> Orion SoCs have edge or level triggered cause registers. That should >> reveal, if it is more sane to use handle_edge_irq here and possibly in >> the main interrupt controller, too. > > There is a mixture. > > Any cause bit documented to be clearable is edge triggered, all others > are level. > > On Kirkwood this means all of the main interrupt controller bits are > level and all the bridge bits are edge. Which means edge is > definitely correct for the bridge handler, and level correct for the > main handler. Just checked that for Dove, it is the same there. Main IRQ_CAUSE is RO, BRIDGE_CAUSE is RW0C, and PMU_CAUSE is RW *sigh*. I need to remember that when Dove moves over to mach-mvebu, as we need a different chained irq handler for PMU that deals with that broken RW register. Sebastian
On Wed, Jan 22, 2014 at 05:19:34PM -0700, Jason Gunthorpe wrote: [..] > > On Kirkwood this means all of the main interrupt controller bits are > level and all the bridge bits are edge. Which means edge is > definitely correct for the bridge handler, and level correct for the > main handler. > Which means we should change the bridge handler to edge? Is there any _noticeable_ side-effect in doing the handling via level?
diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c index e51d400..91a3955 100644 --- a/drivers/irqchip/irq-orion.c +++ b/drivers/irqchip/irq-orion.c @@ -108,6 +108,16 @@ IRQCHIP_DECLARE(orion_intc, "marvell,orion-intc", orion_irq_init); #define ORION_BRIDGE_IRQ_CAUSE 0x00 #define ORION_BRIDGE_IRQ_MASK 0x04 +static unsigned int orion_bridge_irq_startup(struct irq_data *data) +{ + /* Ack pending interrupts */ + data->chip->irq_ack(data); + + /* Unmask the interrupt */ + data->chip->irq_unmask(data); + return 0; +} + static void orion_bridge_irq_handler(unsigned int irq, struct irq_desc *desc) { struct irq_domain *d = irq_get_handler_data(irq); @@ -176,6 +186,7 @@ static int __init orion_bridge_irq_init(struct device_node *np, gc->chip_types[0].regs.ack = ORION_BRIDGE_IRQ_CAUSE; gc->chip_types[0].regs.mask = ORION_BRIDGE_IRQ_MASK; + gc->chip_types[0].chip.irq_startup = orion_bridge_irq_startup; gc->chip_types[0].chip.irq_ack = irq_gc_ack_clr_bit; gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit; gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;