| Message ID | 1370536034-23956-5-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Sebastian Hesselbarth, On Thu, 6 Jun 2013 18:27:12 +0200, Sebastian Hesselbarth wrote: > - wdt@20300 { > + wdt: watchdog-timer@20300 { > compatible = "marvell,orion-wdt"; > reg = <0x20300 0x28>; > + interrupt-parent = <&bridge_intc>; > + interrupts = <3>; > clocks = <&gate_clk 7>; > status = "okay"; > }; The watchdog driver is mapping the same registers as the timer driver (0x20300) and is poking into the same TIMER_CTRL register that controls both the timers and the watchdog. In addition to this, the watchdog driver also pokes into some other registers, such as BRIDGE_CAUSE and RSTOUTn_MASK. As we want to bring watchdog support for Armada 370/XP, I'm wondering if we should fix those problems, and if yes, how: (1) The timer driver is also responsible for handling the watchdog (probably the easiest solution) (2) Have some sort of 'common code' between the timer driver and the watchdog driver to control the access to the shared TIMER_CTRL register. (3) Something else. And this still does not solve the access to BRIDGE_CAUSE and RSTOUTn_MASK. Ideas? Thomas
On 06/07/13 10:30, Thomas Petazzoni wrote: > On Thu, 6 Jun 2013 18:27:12 +0200, Sebastian Hesselbarth wrote: >> - wdt@20300 { >> + wdt: watchdog-timer@20300 { >> compatible = "marvell,orion-wdt"; >> reg = <0x20300 0x28>; >> + interrupt-parent = <&bridge_intc>; >> + interrupts = <3>; >> clocks = <&gate_clk 7>; >> status = "okay"; >> }; > > The watchdog driver is mapping the same registers as the timer driver > (0x20300) and is poking into the same TIMER_CTRL register that controls > both the timers and the watchdog. Thomas, you are right. I must admit that I forgot to take care of watchdog driver. > In addition to this, the watchdog driver also pokes into some other > registers, such as BRIDGE_CAUSE and RSTOUTn_MASK. As you can see above, watchdog should depend on chained interrupts but current implementation doesn't but clears itself in BRIDGE_CAUSE. Current non-DT timer also does (thread unsafe). DT timer depends on the chained irq handler introduced with this patch set. So for the interrupt, watchdog should also depend on the chained irq handler to clear wdt irq. Access to TIMER_CTRL should be made thread safe. I suggest to put that common code into orion clocksource as it will be always compiled in while wdt is optional. > As we want to bring watchdog support for Armada 370/XP, I'm wondering > if we should fix those problems, and if yes, how: > > (1) The timer driver is also responsible for handling the watchdog > (probably the easiest solution) Well, there is drivers/watchdog where current (Orion) wdt is located. I guess it should stay there. For Armada 370/XP I guess it will need to clear the watchdog events in common timer registers as for the timer events? That is why I didn't merge Orion clocksource into Armada 370/XP clocksource because we would have to check for Orion/Armada 370/XP on every timer event. > (2) Have some sort of 'common code' between the timer driver and the > watchdog driver to control the access to the shared TIMER_CTRL > register. Yes. Both should call a common thread-safe timer_en(num) at least. > (3) Something else. > > And this still does not solve the access to BRIDGE_CAUSE and > RSTOUTn_MASK. BRIDGE_CAUSE is taken care of by making wdt depend on chained irq handler.. RSTOUTn_MASK is only used by current common code on reset, maybe there is an API for that I have missed yet. But both reset and watchdog will ultimately cause a reset - maybe we can accept that for now. Sebastian
On 06/07/13 10:30, Thomas Petazzoni wrote: > On Thu, 6 Jun 2013 18:27:12 +0200, Sebastian Hesselbarth wrote: > >> - wdt@20300 { >> + wdt: watchdog-timer@20300 { >> compatible = "marvell,orion-wdt"; >> reg = <0x20300 0x28>; >> + interrupt-parent = <&bridge_intc>; >> + interrupts = <3>; >> clocks = <&gate_clk 7>; >> status = "okay"; >> }; > > The watchdog driver is mapping the same registers as the timer driver > (0x20300) and is poking into the same TIMER_CTRL register that controls > both the timers and the watchdog. Thomas, I didn't comment on the base address above: with issue (b) solved the actual reg property of wdt should be <0x20320 0x8> while timer's reg property is <0x20300 0x20>. I had a closer look at orion-wdt. Actually, I don't want to poke into it too much, but DT irqchip introduces that chained irq handler for bridge registers. While clocksource allows us to have separate drivers for DT and non-DT, current watchdog does not. This introduces some issues to take care of when dealing with kernels where you have both non-DT and DT compiled in. (a) non-DT timer and DT/non-DT watchdog poke BRIDGE_CAUSE Convert non-DT irq to install chained irq and remove it from non-DT timer and DT/non-DT watchdog. (b) non-DT timer, DT timer, and DT/non-DT watchdog poke TIMER_CTRL Have non-DT timer and DT timer export an orion_timer_ctrl_clrset() function that spin_locks access to TIMER_CTRL register. Make non-DT timer, DT timer, and DT/non-DT watchdog use that exported function. One thing we need to workaround here: You cannot have the same function name exported from both non-DT and DT timer. For a temporary "fix" this function could sit in arch/arm/plat-orion/temporary.c until we get rid of non-DT completely. Maybe we also want some place there to subsequently split-off code from common.c when we switch to other APIs for DT kernels (Reset API). (c) common plat-orion reset and watchdog poke RSTOUTn_MASK Leave it. *OR* (d) implement a DT-only version of watchdog From what I can see from current orion wdt I prefer forking the whole driver and remove of_device_id from the old one. Or just have non-DT and DT callbacks where required. That will safe us from messing with non-DT core drivers just for getting orion wdt to behave on DT kernels. And just for the record, we fork off new drivers for DT all the time but watchdog already sits in drivers/ while the rest moves from arch/arm to drivers/. Sebastian
diff --git a/arch/arm/boot/dts/kirkwood.dtsi b/arch/arm/boot/dts/kirkwood.dtsi index c0c4811..ca296c3 100644 --- a/arch/arm/boot/dts/kirkwood.dtsi +++ b/arch/arm/boot/dts/kirkwood.dtsi @@ -8,13 +8,6 @@ gpio0 = &gpio0; gpio1 = &gpio1; }; - intc: interrupt-controller { - compatible = "marvell,orion-intc", "marvell,intc"; - interrupt-controller; - #interrupt-cells = <1>; - reg = <0xf1020204 0x04>, - <0xf1020214 0x04>; - }; ocp@f1000000 { compatible = "simple-bus"; @@ -24,6 +17,30 @@ #address-cells = <1>; #size-cells = <1>; + timer: timer@20300 { + compatible = "marvell,orion-timer"; + reg = <0x20300 0x20>; + interrupt-parent = <&bridge_intc>; + interrupts = <1>, <2>; + clocks = <&core_clk 0>; + }; + + intc: main-interrupt-ctrl@20200 { + compatible = "marvell,orion-intc"; + interrupt-controller; + #interrupt-cells = <1>; + reg = <0x20200 0x10>, <0x20210 0x10>; + }; + + bridge_intc: bridge-interrupt-ctrl@20110 { + compatible = "marvell,orion-bridge-intc"; + interrupt-controller; + #interrupt-cells = <1>; + reg = <0x20110 0x8>; + interrupts = <1>; + marvell,#interrupts = <6>; + }; + core_clk: core-clocks@10030 { compatible = "marvell,kirkwood-core-clock"; reg = <0x10030 0x4>; @@ -97,9 +114,11 @@ #clock-cells = <1>; }; - wdt@20300 { + wdt: watchdog-timer@20300 { compatible = "marvell,orion-wdt"; reg = <0x20300 0x28>; + interrupt-parent = <&bridge_intc>; + interrupts = <3>; clocks = <&gate_clk 7>; status = "okay"; };
With recent support for true irqchip and clocksource drivers for Orion SoCs, now make use of it on DT enabled Kirkwood boards. Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> --- Cc: Grant Likely <grant.likely@linaro.org> Cc: Rob Herring <rob.herring@calxeda.com> Cc: Rob Landley <rob@landley.net> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: John Stultz <john.stultz@linaro.org> Cc: Russell King <linux@arm.linux.org.uk> Cc: Jason Cooper <jason@lakedaemon.net> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Gregory Clement <gregory.clement@free-electrons.com> Cc: devicetree-discuss@lists.ozlabs.org Cc: linux-doc@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org --- arch/arm/boot/dts/kirkwood.dtsi | 35 +++++++++++++++++++++++++++-------- 1 files changed, 27 insertions(+), 8 deletions(-)