Re: [PATCH v3 4/6] ARM: kirkwood: move device tree nodes to DT irqchip and clocksource
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
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 4/6] ARM: kirkwood: move device tree nodes to DT irqchip and clocksource
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
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v3 4/6] ARM: kirkwood: move device tree nodes to DT irqchip and clocksource
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
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH v3 4/6] ARM: kirkwood: move device tree nodes to DT irqchip and clocksource
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.hesselba...@gmail.com
---
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Rob Landley r...@landley.net
Cc: Thomas Gleixner t...@linutronix.de
Cc: John Stultz john.stu...@linaro.org
Cc: Russell King li...@arm.linux.org.uk
Cc: Jason Cooper ja...@lakedaemon.net
Cc: Andrew Lunn and...@lunn.ch
Cc: Thomas Petazzoni thomas.petazz...@free-electrons.com
Cc: Gregory Clement gregory.clem...@free-electrons.com
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-ker...@vger.kernel.org
---
arch/arm/boot/dts/kirkwood.dtsi | 35 +++
1 files changed, 27 insertions(+), 8 deletions(-)
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@f100 {
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;
};
--
1.7.2.5
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
