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 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 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 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 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 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 = _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/
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/
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/
[PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer
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
Tested on a Kirkwood platform.
NOTE: I'm skeptical that MV78xx0 has a bridge interrupt cause/mask
register. However, it was setup so the timer code would touch those
registers, so I've preserved that. Unfortunately prior to this patch
the 'bridge cause register' was only written to, never read. If it is
wired-to-zero on MV78xx0 because it doesn't exist then the timer will
fail to function. The fix is easy, but I need someone with the
manual/system to tell me which is right.
Signed-off-by: Jason Gunthorpe
---
arch/arm/mach-dove/common.c |5 +-
arch/arm/mach-dove/include/mach/bridge-regs.h |2 +-
arch/arm/mach-dove/include/mach/irqs.h|9 +++-
arch/arm/mach-kirkwood/common.c |6 ++-
arch/arm/mach-kirkwood/include/mach/bridge-regs.h |2 -
arch/arm/mach-kirkwood/include/mach/irqs.h| 14 +-
arch/arm/mach-mv78xx0/common.c|8 +++-
arch/arm/mach-mv78xx0/include/mach/bridge-regs.h |2 +-
arch/arm/mach-mv78xx0/include/mach/irqs.h |9 +++-
arch/arm/mach-orion5x/common.c|6 ++-
arch/arm/mach-orion5x/include/mach/bridge-regs.h |2 -
arch/arm/mach-orion5x/include/mach/irqs.h |9 +++-
arch/arm/plat-orion/include/plat/irq.h|3 +
arch/arm/plat-orion/include/plat/time.h |4 +-
arch/arm/plat-orion/irq.c | 51 +
arch/arm/plat-orion/time.c| 46 ---
16 files changed, 120 insertions(+), 58 deletions(-)
My immediate need is to have the kernel panic on watchdog timer
expiry, but I also think this might be the first step to clean up this
item:
/*
* Disable propagation of mbus errors to the CPU local bus,
* as this causes mbus errors (which can occur for example
* for PCI aborts) to throw CPU aborts, which we're not set
* up to deal with.
*/
writel(readl(CPU_CONFIG) & ~CPU_CONFIG_ERROR_PROP, CPU_CONFIG);
Regards,
Jason
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index f723fe1..9107808 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -243,8 +243,9 @@ static int __init dove_find_tclk(void)
static void __init dove_timer_init(void)
{
dove_tclk = dove_find_tclk();
- orion_time_init(BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
- IRQ_DOVE_BRIDGE, dove_tclk);
+ orion_bridge_irq_init(IRQ_DOVE_BRIDGE, IRQ_DOVE_BRIDGE_START,
+ BRIDGE_CAUSE);
+ orion_time_init(IRQ_DOVE_BRIDGE_TIMER1, dove_tclk);
}
struct sys_timer dove_timer = {
diff --git a/arch/arm/mach-dove/include/mach/bridge-regs.h
b/arch/arm/mach-dove/include/mach/bridge-regs.h
index 99f259e..3bd4656 100644
--- a/arch/arm/mach-dove/include/mach/bridge-regs.h
+++ b/arch/arm/mach-dove/include/mach/bridge-regs.h
@@ -26,7 +26,7 @@
#define SYSTEM_SOFT_RESET (BRIDGE_VIRT_BASE + 0x010c)
#define SOFT_RESET0x0001
-#define BRIDGE_INT_TIMER1_CLR (~0x0004)
+#define BRIDGE_CAUSE(BRIDGE_VIRT_BASE + 0x0110)
#define IRQ_VIRT_BASE (BRIDGE_VIRT_BASE + 0x0200)
#define IRQ_CAUSE_LOW_OFF 0x
diff --git a/arch/arm/mach-dove/include/mach/irqs.h
b/arch/arm/mach-dove/include/mach/irqs.h
index 03d401d..53c7d82 100644
--- a/arch/arm/mach-dove/include/mach/irqs.h
+++ b/arch/arm/mach-dove/include/mach/irqs.h
@@ -78,9 +78,16 @@
#define IRQ_DOVE_SATA 62
/*
+ * Bridge Interrupt Controller
+ */
+#define IRQ_DOVE_BRIDGE_START 64
+#define IRQ_DOVE_BRIDGE_TIMER1 (IRQ_DOVE_BRIDGE_START + 2)
+#define NR_BRIDGE_IRQS 6
+
+/*
* DOVE General Purpose Pins
*/
-#define IRQ_DOVE_GPIO_START64
+#define IRQ_DOVE_GPIO_START(IRQ_DOVE_BRIDGE_START + NR_BRIDGE_IRQS)
#define NR_GPIO_IRQS 64
/*
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index 906c22e..6ec60b8 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include "common.h"
/*
@@ -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,
[PATCH] ARM: Orion: Hoist bridge interrupt handling out of the timer
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
Tested on a Kirkwood platform.
NOTE: I'm skeptical that MV78xx0 has a bridge interrupt cause/mask
register. However, it was setup so the timer code would touch those
registers, so I've preserved that. Unfortunately prior to this patch
the 'bridge cause register' was only written to, never read. If it is
wired-to-zero on MV78xx0 because it doesn't exist then the timer will
fail to function. The fix is easy, but I need someone with the
manual/system to tell me which is right.
Signed-off-by: Jason Gunthorpe jguntho...@obsidianresearch.com
---
arch/arm/mach-dove/common.c |5 +-
arch/arm/mach-dove/include/mach/bridge-regs.h |2 +-
arch/arm/mach-dove/include/mach/irqs.h|9 +++-
arch/arm/mach-kirkwood/common.c |6 ++-
arch/arm/mach-kirkwood/include/mach/bridge-regs.h |2 -
arch/arm/mach-kirkwood/include/mach/irqs.h| 14 +-
arch/arm/mach-mv78xx0/common.c|8 +++-
arch/arm/mach-mv78xx0/include/mach/bridge-regs.h |2 +-
arch/arm/mach-mv78xx0/include/mach/irqs.h |9 +++-
arch/arm/mach-orion5x/common.c|6 ++-
arch/arm/mach-orion5x/include/mach/bridge-regs.h |2 -
arch/arm/mach-orion5x/include/mach/irqs.h |9 +++-
arch/arm/plat-orion/include/plat/irq.h|3 +
arch/arm/plat-orion/include/plat/time.h |4 +-
arch/arm/plat-orion/irq.c | 51 +
arch/arm/plat-orion/time.c| 46 ---
16 files changed, 120 insertions(+), 58 deletions(-)
My immediate need is to have the kernel panic on watchdog timer
expiry, but I also think this might be the first step to clean up this
item:
/*
* Disable propagation of mbus errors to the CPU local bus,
* as this causes mbus errors (which can occur for example
* for PCI aborts) to throw CPU aborts, which we're not set
* up to deal with.
*/
writel(readl(CPU_CONFIG) ~CPU_CONFIG_ERROR_PROP, CPU_CONFIG);
Regards,
Jason
diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index f723fe1..9107808 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -243,8 +243,9 @@ static int __init dove_find_tclk(void)
static void __init dove_timer_init(void)
{
dove_tclk = dove_find_tclk();
- orion_time_init(BRIDGE_VIRT_BASE, BRIDGE_INT_TIMER1_CLR,
- IRQ_DOVE_BRIDGE, dove_tclk);
+ orion_bridge_irq_init(IRQ_DOVE_BRIDGE, IRQ_DOVE_BRIDGE_START,
+ BRIDGE_CAUSE);
+ orion_time_init(IRQ_DOVE_BRIDGE_TIMER1, dove_tclk);
}
struct sys_timer dove_timer = {
diff --git a/arch/arm/mach-dove/include/mach/bridge-regs.h
b/arch/arm/mach-dove/include/mach/bridge-regs.h
index 99f259e..3bd4656 100644
--- a/arch/arm/mach-dove/include/mach/bridge-regs.h
+++ b/arch/arm/mach-dove/include/mach/bridge-regs.h
@@ -26,7 +26,7 @@
#define SYSTEM_SOFT_RESET (BRIDGE_VIRT_BASE + 0x010c)
#define SOFT_RESET0x0001
-#define BRIDGE_INT_TIMER1_CLR (~0x0004)
+#define BRIDGE_CAUSE(BRIDGE_VIRT_BASE + 0x0110)
#define IRQ_VIRT_BASE (BRIDGE_VIRT_BASE + 0x0200)
#define IRQ_CAUSE_LOW_OFF 0x
diff --git a/arch/arm/mach-dove/include/mach/irqs.h
b/arch/arm/mach-dove/include/mach/irqs.h
index 03d401d..53c7d82 100644
--- a/arch/arm/mach-dove/include/mach/irqs.h
+++ b/arch/arm/mach-dove/include/mach/irqs.h
@@ -78,9 +78,16 @@
#define IRQ_DOVE_SATA 62
/*
+ * Bridge Interrupt Controller
+ */
+#define IRQ_DOVE_BRIDGE_START 64
+#define IRQ_DOVE_BRIDGE_TIMER1 (IRQ_DOVE_BRIDGE_START + 2)
+#define NR_BRIDGE_IRQS 6
+
+/*
* DOVE General Purpose Pins
*/
-#define IRQ_DOVE_GPIO_START64
+#define IRQ_DOVE_GPIO_START(IRQ_DOVE_BRIDGE_START + NR_BRIDGE_IRQS)
#define NR_GPIO_IRQS 64
/*
diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index 906c22e..6ec60b8 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -35,6 +35,7 @@
#include plat/time.h
#include plat/addr-map.h
#include linux/platform_data/dma-mv_xor.h
+#include plat/irq.h
#include common.h
/*
@@ -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,
-
