Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of thetimer

[Jason Gunthorpe]

On Sat, Dec 08, 2012 at 12:26:24PM +0100, Andrew Lunn wrote:
 
> 1) It should have an IRQ domain, like the other IRQ chips we have.
> 2) It should have a DT binding, like the other IRQ chips we have.

I was going to look at a DT binding for this as a follow on, since
I'll want to bind to these interrupts. 

Are you OK with keeping this patch as is and seeing DT in a follow up,
or as a series? It is already pretty big.

> 4) We than pass the watchdog interrupt via DT.

Right now the watchdog driver is coded to cause a board reset, so it
doesn't use interrupts at all. Adding interrupt support to watchdog
seems orthogonal to this?

What would it look like? For my boards I want the watchdog to panic(),
because I have another watchdog that takes care of reset, but that
won't be universal.

> We plan to remove old style platforms in the next few cycles, so its

Yay :)

> 3) We then pass the timer interrupt via DT to the timer driver.
> 3) is not so simple, because we currently don't have a timer binding
> for Orion SoC. However, with this cleanup, we are much closer to being
> able to use the 370/XP timer code.

Interesting.. The 370/XP is a more advanced version of the same timer
IP, there are several registers that driver is touching that are not
HW supported, at least on kirkwood.

It would be straightforward to add a binding in the style of
time-armada-370-xp.c, I can probably send that as a third patch.

The two DT bindings are straightforward, and my testing on Kirkwood
should cover alot - but it would be great if non-kirkwood boards could
review/test with this patch..

Do you expect a DT conversion for all orion_time_init users, or just
the one I can test or ..?

> > @@ -534,8 +535,9 @@ static void __init kirkwood_timer_init(void)
> >  {
> >  	kirkwood_tclk = kirkwood_find_tclk();
> >  
> > -	orion_time_init(BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
> > -			IRQ_KIRKWOOD_BRIDGE, kirkwood_tclk);
> > +	orion_bridge_irq_init(IRQ_KIRKWOOD_BRIDGE, IRQ_KIRKWOOD_BRIDGE_START,
> > +			      BRIDGE_CAUSE);
> > +	orion_time_init(IRQ_KIRKWOOD_BRIDGE_TIMER1, kirkwood_tclk);
> >  }
> 
> I think it would be better to do this in kirkwood_init_irq(). Same for
> the other platforms. 

Yes.. I left it like this because it is very easy to audit that it is
correct (ie called, and called at the correc time). When DT support is
added this will have to change again, so expecting that the next patch
will have to change things so orion_bridge_irq_init is not called for
the DT case, and the patch after so orion_time_init is not called for
the DT case, are you OK with this patch leaving it here?

> > +	u32 cause;
> > +	int i;
> > +
> > +	cause = readl(gc->reg_base) & readl(gc->reg_base + 4);
> 
> Could you add a #define for this 4. I guess its an interrupt enable
> mask? Could regs.mask be used?

I will add a define, regs.mask could be used, but since the value is
known and constant I left it as a constant as a micro-optimization.

> > +	gc = irq_alloc_generic_chip("orion_irq_edge", 1, irq_start,
> 
> Maybe the name orion_bridge would be better?

Sure

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/