Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer
On 06/04/13 19:26, Jason Gunthorpe wrote: On Sun, Dec 09, 2012 at 02:06:48PM +0100, Sebastian Hesselbarth wrote: The main irq controller will be required for sure, but for the secondary irq controller we had a discussion long ago. IIRC Gregory proposed to have shared irqs handled by timer and watchdog, I was proposing chained irqs. +1 on decoded IRQs for bridge. I've been running that configuration now since this patch set was first posted. There is too much HW variance, the timer, watchdog, etc drivers should not have to poke into SOC specific registers just to get an interrupt. The bridge decode can either be via a chained handler, or by incorporating the bridge decode into the main kirkwood handler - the latter having lower overhead for timer ticks. Jason, I have irqchip and clocksource drivers done for Orion, just need to find some time to rebase them. For mvebu archs, IIRC, we have wrt to timer-related irqs: - Armada 370/XP with different irq handler and timer irq handling within timer registers. - Orion SoCs with Bridge irq registers for timer related stuff (timer0/1) - Kirkwood and Dove with watchdog timers (both with wdt irq in bridge irqs) - RTC in bridge irqs, but Dove with RTC connected to PMU irqs I think we should have patches for irqchip-orion first and then rethink if we want a standalone timer-orion or merge it with timer-mvebu. Having watchdog using irqs is kind of independent from this. I suggest not to merge clocksource for Orion and Armada 370/XP. They are different enough to justify separate drivers. IIRC Armada 370/XP acks timer interrupts by clearing timer register bits that are not implemented in Orion SoCs. I would think the logical progression is: - irq-chip orion combined with work to keep the existing timer working - Patch to add the bridge irq-chip - Patches to support orion/kirkwood/dove/etc in the existing timer drivers - Patch to update the DT to switch to the bridge and updated timer - Patch to remove the old timer I'd rather have irqchip and clocksource mainlined and enable both drivers when they have surfaced. I try to sent patches by end of this week. When I last looked briefly, it seems like merging with timer-mvebu was fairly straightforward.. Back in the days when Gregory, Thomas, and I were looking into merged timer we agreed not to have an extra check on 25MHz support. If you put the property in the node, it will try to set the timer to fixed 25MHz. If you use the property on Orion timer, it will just break timer handling. As for the mveth case we should have a compatible tag for each SOC, the driver can ignore it, but it should be in the DT for future use.. We could have a single clocksource driver but as said above, clocksource is a tiny driver compared to others. Separate drivers will save us from checking SoC on every timer event or have a callback for Armada 370/XP clearing timer irqs. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer
On Sun, Dec 09, 2012 at 02:06:48PM +0100, Sebastian Hesselbarth wrote: > The main irq controller will be required for sure, but for the secondary > irq controller we had a discussion long ago. IIRC Gregory proposed to have > shared irqs handled by timer and watchdog, I was proposing chained irqs. +1 on decoded IRQs for bridge. I've been running that configuration now since this patch set was first posted. There is too much HW variance, the timer, watchdog, etc drivers should not have to poke into SOC specific registers just to get an interrupt. The bridge decode can either be via a chained handler, or by incorporating the bridge decode into the main kirkwood handler - the latter having lower overhead for timer ticks. > For mvebu archs, IIRC, we have wrt to timer-related irqs: > - Armada 370/XP with different irq handler and timer irq handling within > timer registers. > - Orion SoCs with Bridge irq registers for timer related stuff (timer0/1) > - Kirkwood and Dove with watchdog timers (both with wdt irq in bridge irqs) > - RTC in bridge irqs, but Dove with RTC connected to PMU irqs > I think we should have patches for irqchip-orion first and then rethink > if we want a standalone timer-orion or merge it with timer-mvebu. Having > watchdog using irqs is kind of independent from this. I would think the logical progression is: - irq-chip orion combined with work to keep the existing timer working - Patch to add the bridge irq-chip - Patches to support orion/kirkwood/dove/etc in the existing timer drivers - Patch to update the DT to switch to the bridge and updated timer - Patch to remove the old timer When I last looked briefly, it seems like merging with timer-mvebu was fairly straightforward.. > Back in the days when Gregory, Thomas, and I were looking into merged timer > we agreed not to have an extra check on 25MHz support. If you put the > property in the node, it will try to set the timer to fixed 25MHz. If you > use the property on Orion timer, it will just break timer handling. As for the mveth case we should have a compatible tag for each SOC, the driver can ignore it, but it should be in the DT for future use.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer
On 12/09/2012 09:30 AM, Andrew Lunn wrote: On Sat, Dec 08, 2012 at 07:57:48PM -0700, Jason Gunthorpe wrote: 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. Hi Jason A patch series is great. However, its not so good practice to add something on the first patch, and then move it somewhere else in the next. So i would suggest initializing the controller in kirkwood_irq_init(), etc as its added. Hi Andrew, Jason, first of all I have adjusted the Cc list a little bit: Gregory is missing, and I removed Mike and Olof as they might not be that interested in discussing mvebu internals. Please re-add if I am wrong. I have an irqchip driver for orion that I wrote some weeks ago. I can prepare a patch today. I will allow to have main irq controller by DT and I can also add a secondary irq controller for the bridge registers as chained irq handler. If I dig hard enough in my branches I will also find time-orion drivers, that are either "standalone" (i.e. orion only) or merged with time-mvebu. The main irq controller will be required for sure, but for the secondary irq controller we had a discussion long ago. IIRC Gregory proposed to have shared irqs handled by timer and watchdog, I was proposing chained irqs. For mvebu archs, IIRC, we have wrt to timer-related irqs: - Armada 370/XP with different irq handler and timer irq handling within timer registers. - Orion SoCs with Bridge irq registers for timer related stuff (timer0/1) - Kirkwood and Dove with watchdog timers (both with wdt irq in bridge irqs) - RTC in bridge irqs, but Dove with RTC connected to PMU irqs I think we should have patches for irqchip-orion first and then rethink if we want a standalone timer-orion or merge it with timer-mvebu. Having watchdog using irqs is kind of independent from this. ... 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. Yes, the 25MHz and the divider for example. I'm not 100% sure it will actually work, it will need a different compatibility string, and a bit of configuration based on that string, but i think it goes. If you compare the two different drivers, they are very similar. Back in the days when Gregory, Thomas, and I were looking into merged timer we agreed not to have an extra check on 25MHz support. If you put the property in the node, it will try to set the timer to fixed 25MHz. If you use the property on Orion timer, it will just break timer handling. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer
On Sat, Dec 08, 2012 at 07:57:48PM -0700, Jason Gunthorpe wrote: > 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. Hi Jason A patch series is great. However, its not so good practice to add something on the first patch, and then move it somewhere else in the next. So i would suggest initializing the controller in kirkwood_irq_init(), etc as its added. > > 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? Yes, its orthogonal, but a logical extension which could be part of a patchset. > 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. There are examples of watchdogs that allow this. However, none yet have DT bindings. I would suggest adding an optional property, "panic", which indicates the driver should panic rather than reboot. Make sure to run this by the device tree mailing list. > > 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. Yes, the 25MHz and the divider for example. I'm not 100% sure it will actually work, it will need a different compatibility string, and a bit of configuration based on that string, but i think it goes. If you compare the two different drivers, they are very similar. > 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 ..? Please take a stab at converting them all. We have an active set of testers. I can test kirkwood and orion5x, Sebastian tests Dove, Thomas and Gregory test 370/XP if needed. Nobody seems to care about mv78xx0 so its slowly bit-rotting in a corner. Thanks Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer
On Fri, Dec 07, 2012 at 03:55:07PM -0700, Jason Gunthorpe wrote: > The intent of this patch is to expose the other bridge cause > interrupts to users in the kernel. > > - Add orion_bridge_irq_init to create a new edge triggered interrupt > chip based on the bridge cause register > - Remove all interrupt register code from time.c and use normal > interrupt functions instead > - Update the machines that use orion_time_init to call > orion_bridge_irq_init and use the new signature Hi Jason I like the idea, but i think it needs to go a bit further. 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. 3) We then pass the timer interrupt via DT to the timer driver. 4) We than pass the watchdog interrupt via DT. We plan to remove old style platforms in the next few cycles, so its important that changes like this are orientated towards DT but have the necessary backward compatibility for the old ways for the boards not yet converted. I think for Dove all boards are DT based... 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. > @@ -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. > +static void bridge_irq_handler(unsigned irq, struct irq_desc *desc) > +{ > + struct irq_chip_generic *gc = irq_get_handler_data(irq); > + 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? > + for (i = 0; i < 6; i++) > + if ((cause & (1 << i))) > + generic_handle_irq(i + gc->irq_base); > +} > + > +static void irq_gc_eoi_inv(struct irq_data *d) > +{ > + struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > + u32 mask = 1 << (d->irq - gc->irq_base); > + struct irq_chip_regs *regs; > + > + regs = &container_of(d->chip, struct irq_chip_type, chip)->regs; > + irq_gc_lock(gc); > + irq_reg_writel(~mask, gc->reg_base + regs->eoi); > + irq_gc_unlock(gc); > +} > + > +void __init orion_bridge_irq_init(unsigned int bridge_irq, > + unsigned int irq_start, > + void __iomem *causeaddr) > +{ > + struct irq_chip_generic *gc; > + struct irq_chip_type *ct; > + > + gc = irq_alloc_generic_chip("orion_irq_edge", 1, irq_start, Maybe the name orion_bridge would be better? Thanks Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/