Patchwork [v2,06/15] watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

login
register
mail settings
Submitter Ezequiel Garcia
Date Jan. 22, 2014, 10:12 p.m.
Message ID <20140122221237.GA30763@localhost>
Download mbox | patch
Permalink /patch/313427/
State New
Headers show

Comments

Ezequiel Garcia - Jan. 22, 2014, 10:12 p.m.
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?
Jason Gunthorpe - Jan. 22, 2014, 10:31 p.m.
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
Ezequiel Garcia - Jan. 22, 2014, 10:56 p.m.
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.
Sebastian Hesselbarth - Jan. 23, 2014, 12:03 a.m.
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
Jason Gunthorpe - Jan. 23, 2014, 12:19 a.m.
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
Sebastian Hesselbarth - Jan. 23, 2014, 12:35 a.m.
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
Ezequiel Garcia - Jan. 23, 2014, 12:04 p.m.
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?

Patch

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;