Orion Watchdog fixes

This patchset introduces a bunch of fixes that removes the direct use of the shared timer control register, and also removes the need to include a mach-specific header.

With this changes the driver can be included in multiplatforms builds, and in particular be used for the Armada 370/XP SoC.

I'm sending this early patchset mainly to discuss the possibility of introducing this changes. As cleared stated in each patch, this series breaks device-tree compatilibity by changing the meaning of the watchdog node 'reg' property.

I'm aware of the implications of this, but I'm not sure how 'stable' the Kirkwood DT is considered. So, given we're currently moving things around, maybe there is still a chance to do this.

This patchset applies on top of Sebastian Hesselbarth's branch:

git://github.com/shesselba/linux-dove.git orion-irqchip-for-v3.11_v4

Ezequiel Garcia (10):
clocksource: orion: Add thread-safe API header
watchdog: orion: Use thread-safe clocksource API
watchdog: orion: Rename device-tree binding documentation
watchdog: orion: Use the proper watchdog register
watchdog: orion: Add a memory resource for RSTOUT register
watchdog: orion: Update device-tree binding documentation
watchdog: orion: Remove unneeded BRIDGE_CAUSE clear
watchdog: orion: Remove mach-specific unneeded header
watchdog: orion: Use BIT()
ARM: kirkwood: Fix the device-tree watchdog's node reg property

 .../watchdog/{marvel.txt => orion-wdt.txt}         |  8 ++--
 arch/arm/boot/dts/kirkwood.dtsi                    |  3 +-
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h  |  2 +
 arch/arm/mach-orion5x/include/mach/bridge-regs.h   |  2 +
 arch/arm/plat-orion/common.c                       | 10 +++--
 drivers/watchdog/orion_wdt.c                       | 46 +++++++++-------------
 include/linux/time-orion.h                         |  7 ++++
 7 files changed, 42 insertions(+), 36 deletions(-)
 rename Documentation/devicetree/bindings/watchdog/{marvel.txt => orion-wdt.txt} (58%)
 create mode 100644 include/linux/time-orion.h

1.8.1.5

Add a header declaration to allow drivers (such as watchdog) to access this exported API.

---
 include/linux/time-orion.h | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 include/linux/time-orion.h

new file mode 100644
index 0000000..c7e6458
@@ -0,0 +1,7 @@
+
+#ifndef __TIME_ORION_H
+#define __TIME_ORION_H
+
+void orion_timer_ctrl_clrset(u32 clr, u32 set);
+
+#endif

1.8.1.5

The TIMER_CTRL register allows to control timer and watchdog counters, so it's a register shared between the clocksource and the watchdog drivers. In order to prevent race-conditions the clocksource driver exposed a thread-safe API. Use the API.

---
 drivers/watchdog/orion_wdt.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

index da57798..c0597f4 100644
@@ -25,12 +25,12 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/time-orion.h>
 #include <mach/bridge-regs.h>
 
 /*
  * Watchdog timer block registers.
  */
-#define TIMER_CTRL		0x0000
 #define WDT_EN			0x0010
 #define WDT_VAL			0x0024
 

@@ -72,9 +72,7 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	writel(reg, BRIDGE_CAUSE);
 
 	/* Enable watchdog timer */
-	reg = readl(wdt_reg + TIMER_CTRL);
-	reg |= WDT_EN;
-	writel(reg, wdt_reg + TIMER_CTRL);
+	orion_timer_ctrl_clrset(0, WDT_EN);
 
 	/* Enable reset on watchdog */
 	reg = readl(RSTOUTn_MASK);

@@ -97,9 +95,7 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 	writel(reg, RSTOUTn_MASK);
 
 	/* Disable watchdog timer */
-	reg = readl(wdt_reg + TIMER_CTRL);
-	reg &= ~WDT_EN;
-	writel(reg, wdt_reg + TIMER_CTRL);
+	orion_timer_ctrl_clrset(WDT_EN, 0);
 
 	spin_unlock(&wdt_lock);
 	return 0;

1.8.1.5

Name this file to something a bit more judicious.

---
 Documentation/devicetree/bindings/watchdog/{marvel.txt => orion-wdt.txt} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename Documentation/devicetree/bindings/watchdog/{marvel.txt => orion-wdt.txt} (100%)

similarity index 100%
rename from Documentation/devicetree/bindings/watchdog/marvel.txt
rename to Documentation/devicetree/bindings/watchdog/orion-wdt.txt

1.8.1.5

Until now the watchdog driver was using the timer control register to access the watchdog counter register. This is not appropriate, given the timer control register should be controlled by the clocksource driver alone.

Fix this by passing the correct register address to the driver and making direct use of it. Note that this breaks the current device-tree binding compatibility since it changes the meaning of the 'reg' property.

---
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h | 1 +
 arch/arm/mach-orion5x/include/mach/bridge-regs.h  | 1 +
 arch/arm/plat-orion/common.c                      | 2 +-
 drivers/watchdog/orion_wdt.c                      | 7 +++----
 4 files changed, 6 insertions(+), 5 deletions(-)

index d4cbe5e..c3f361d 100644
@@ -40,6 +40,7 @@
 
 #define TIMER_VIRT_BASE		(BRIDGE_VIRT_BASE + 0x0300)
 #define TIMER_PHYS_BASE		(BRIDGE_PHYS_BASE + 0x0300)
+#define WDT_PHYS_BASE		(BRIDGE_PHYS_BASE + 0x0324)
 
 #define L2_CONFIG_REG		(BRIDGE_VIRT_BASE + 0x0128)
 #define L2_WRITETHROUGH		0x00000010

index 461fd69..aa35de3 100644
@@ -36,4 +36,5 @@
 
 #define TIMER_VIRT_BASE		(ORION5X_BRIDGE_VIRT_BASE + 0x300)
 #define TIMER_PHYS_BASE		(ORION5X_BRIDGE_PHYS_BASE + 0x300)
+#define WDT_PHYS_BASE		(ORION5X_BRIDGE_PHYS_BASE + 0x324)
 #endif

index c019b7a..77fac6c 100644
@@ -595,7 +595,7 @@ void __init orion_spi_1_init(unsigned long mapbase)
  * Watchdog
  ****************************************************************************/
 static struct resource orion_wdt_resource =
-		DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x28);
+		DEFINE_RES_MEM(WDT_PHYS_BASE, 0x04);
 
 static struct platform_device orion_wdt_device = {
 	.name		= "orion_wdt",

index c0597f4..01bcf53 100644
@@ -32,7 +32,6 @@
  * Watchdog timer block registers.
  */
 #define WDT_EN			0x0010
-#define WDT_VAL			0x0024
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0

@@ -51,7 +50,7 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	spin_lock(&wdt_lock);
 
 	/* Reload watchdog duration */
-	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
+	writel(wdt_tclk * wdt_dev->timeout, wdt_reg);
 
 	spin_unlock(&wdt_lock);
 	return 0;

@@ -64,7 +63,7 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	spin_lock(&wdt_lock);
 
 	/* Set watchdog duration */
-	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
+	writel(wdt_tclk * wdt_dev->timeout, wdt_reg);
 
 	/* Clear watchdog timer interrupt */
 	reg = readl(BRIDGE_CAUSE);

@@ -106,7 +105,7 @@ static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 	unsigned int time_left;
 
 	spin_lock(&wdt_lock);
-	time_left = readl(wdt_reg + WDT_VAL) / wdt_tclk;
+	time_left = readl(wdt_reg) / wdt_tclk;
 	spin_unlock(&wdt_lock);
 
 	return time_left;

1.8.1.5

Instead of accessing the RSTOUT register directly, this commit adds a platform memory resource to map this register into the driver.

Note that by adding a required 2nd-cell for the reg property, this change breaks the device-tree binding compatibility.

---
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h |  1 +
 arch/arm/mach-orion5x/include/mach/bridge-regs.h  |  1 +
 arch/arm/plat-orion/common.c                      | 10 ++++++----
 drivers/watchdog/orion_wdt.c                      | 18 ++++++++++--------
 4 files changed, 18 insertions(+), 12 deletions(-)

index c3f361d..dead75a 100644
@@ -21,6 +21,7 @@
 #define CPU_RESET		0x00000002
 
 #define RSTOUTn_MASK		(BRIDGE_VIRT_BASE + 0x0108)
+#define RSTOUT_PHYS_BASE	(BRIDGE_PHYS_BASE + 0x0108)
 #define WDT_RESET_OUT_EN	0x00000002
 #define SOFT_RESET_OUT_EN	0x00000004
 

index aa35de3..ccd91c9 100644
@@ -18,6 +18,7 @@
 #define CPU_CTRL		(ORION5X_BRIDGE_VIRT_BASE + 0x104)
 
 #define RSTOUTn_MASK		(ORION5X_BRIDGE_VIRT_BASE + 0x108)
+#define RSTOUT_PHYS_BASE	(ORION5X_BRIDGE_PHYS_BASE + 0x108)
 #define WDT_RESET_OUT_EN	0x0002
 
 #define CPU_SOFT_RESET		(ORION5X_BRIDGE_VIRT_BASE + 0x10c)

index 77fac6c..37924f8 100644
@@ -594,14 +594,16 @@ void __init orion_spi_1_init(unsigned long mapbase)
 /*****************************************************************************
  * Watchdog
  ****************************************************************************/
-static struct resource orion_wdt_resource =
-		DEFINE_RES_MEM(WDT_PHYS_BASE, 0x04);
+static struct resource orion_wdt_resource[] = {
+		DEFINE_RES_MEM(WDT_PHYS_BASE, 0x04),
+		DEFINE_RES_MEM(RSTOUT_PHYS_BASE, 0x04)
+};
 
 static struct platform_device orion_wdt_device = {
 	.name		= "orion_wdt",
 	.id		= -1,
-	.num_resources	= 1,
-	.resource	= &orion_wdt_resource,
+	.num_resources	= ARRAY_SIZE(orion_wdt_resource),
+	.resource	= orion_wdt_resource,
 };
 
 void __init orion_wdt_init(void)

index 01bcf53..3c9b3d2 100644
@@ -43,6 +43,7 @@ static unsigned int wdt_max_duration;	/* (seconds) */
 static struct clk *clk;
 static unsigned int wdt_tclk;
 static void __iomem *wdt_reg;
+static void __iomem *rstout_reg;
 static DEFINE_SPINLOCK(wdt_lock);
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)

@@ -74,9 +75,7 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	orion_timer_ctrl_clrset(0, WDT_EN);
 
 	/* Enable reset on watchdog */
-	reg = readl(RSTOUTn_MASK);
-	reg |= WDT_RESET_OUT_EN;
-	writel(reg, RSTOUTn_MASK);
+	writel(readl(rstout_reg) | WDT_RESET_OUT_EN, rstout_reg);
 
 	spin_unlock(&wdt_lock);
 	return 0;

@@ -84,14 +83,10 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 
 static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 {
-	u32 reg;
-
 	spin_lock(&wdt_lock);
 
 	/* Disable reset on watchdog */
-	reg = readl(RSTOUTn_MASK);
-	reg &= ~WDT_RESET_OUT_EN;
-	writel(reg, RSTOUTn_MASK);
+	writel(readl(rstout_reg) & ~WDT_RESET_OUT_EN, rstout_reg);
 
 	/* Disable watchdog timer */
 	orion_timer_ctrl_clrset(WDT_EN, 0);

@@ -158,6 +153,13 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!wdt_reg)
 		return -ENOMEM;
 
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res)
+		return -ENODEV;
+	rstout_reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!rstout_reg)
+		return -ENOMEM;
+
 	wdt_max_duration = WDT_MAX_CYCLE_COUNT / wdt_tclk;
 
 	orion_wdt.timeout = wdt_max_duration;

1.8.1.5

Hi Ezequiel

Have you looked at:

arch/arm/mach-mvebu/system-controller.c

It is also using this register. Are we going to have a similar problem as the TIMER_CTRL register, which you refactered in an earlier patch?

"marvell,orion-system-controller" is not actually used yet, but once kirkwood moves into mach-mvebu, it will start using it.

Andrew

Andrew,

Mmm... I saw the use of the RSTOUT register in kirkwood_restart() but wasn't sure who should be the real 'owner' of this register.

Probably.

I guess so. We should take that into account *now*. Let me think about it and see if I can have something sane for v2.

Thanks!

Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Now that the 'reg' property meaning has been changed, this commit updates the deivce-tree binding documentation.

---
 Documentation/devicetree/bindings/watchdog/orion-wdt.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

index 5dc8d30..48a71fc 100644
@@ -3,7 +3,8 @@
 Required Properties:
 
 - Compatibility : "marvell,orion-wdt"
-- reg		: Address of the timer registers
+- reg		: First cell contains the register for the watchdog counter.
+		  Second cell contains the register for the RSTOUT mask.
 
 Optional properties:
 

@@ -11,9 +12,10 @@ Optional properties:
 
 Example:
 
-	wdt at 20300 {
+	wdt at 20324 {
 		compatible = "marvell,orion-wdt";
-		reg = <0x20300 0x28>;
+		reg = <0x20324 0x4
+		       0x20108 0x4>;
 		timeout-sec = <10>;
 		status = "okay";
 	};

1.8.1.5

nit. s/deivce/device/

With the introduction of the orion irqchip driver, now the BRIDGE_CAUSE bit is cleared by it. There's no longer a need to do it in the watchdog driver, so we can simply remove it.

---
 drivers/watchdog/orion_wdt.c | 7 -------
 1 file changed, 7 deletions(-)

index 3c9b3d2..4a0ab47 100644
@@ -59,18 +59,11 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 
 static int orion_wdt_start(struct watchdog_device *wdt_dev)
 {
-	u32 reg;
-
 	spin_lock(&wdt_lock);
 
 	/* Set watchdog duration */
 	writel(wdt_tclk * wdt_dev->timeout, wdt_reg);
 
-	/* Clear watchdog timer interrupt */
-	reg = readl(BRIDGE_CAUSE);
-	reg &= ~WDT_INT_REQ;
-	writel(reg, BRIDGE_CAUSE);
-
 	/* Enable watchdog timer */
 	orion_timer_ctrl_clrset(0, WDT_EN);
 

1.8.1.5

The mach/bridge-regs.h header is not needed anymore, so we can remove it. This commit allows to use this driver on multiplatforms builds.

---
 drivers/watchdog/orion_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4a0ab47..4ed7a74 100644
@@ -26,12 +26,12 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #include <linux/time-orion.h>
-#include <mach/bridge-regs.h>
 
 /*
  * Watchdog timer block registers.
  */
 #define WDT_EN			0x0010
+#define WDT_RESET_OUT_EN	0x0002
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0

1.8.1.5

This is a purely cosmetic commit: we replace hardcoded values that representing bits by BIT(), which is slightly more readable.

---
 drivers/watchdog/orion_wdt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 4ed7a74..ae64cec 100644
@@ -30,8 +30,8 @@
 /*
  * Watchdog timer block registers.
  */
-#define WDT_EN			0x0010
-#define WDT_RESET_OUT_EN	0x0002
+#define WDT_EN			BIT(4)
+#define WDT_RESET_OUT_EN	BIT(1)
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0

1.8.1.5

The watchdog driver now needs two 'reg' property cells. The first one is for the register containing the watchdog counter, while the second one is for the RSTOUT register.

---
 arch/arm/boot/dts/kirkwood.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index ca296c3..0d74ee9 100644
@@ -116,7 +116,8 @@
 
 		wdt: watchdog-timer at 20300 {
 			compatible = "marvell,orion-wdt";
-			reg = <0x20300 0x28>;
+			reg = <0x20324 0x04
+			       0x20108 0x04>;
 			interrupt-parent = <&bridge_intc>;
 			interrupts = <3>;
 			clocks = <&gate_clk 7>;

1.8.1.5

Hi Ezequiel

Maybe i'm missing something here. You are making use of orion_timer_ctrl_clrset() from time-orion.c. How will this work on 370/XP which has a different clocksource driver?

Thanks
Andrew

Dear Andrew Lunn,

I *think* the idea is that the Armada 370/XP driver will expose the same function, so from the point of view of the watchdog driver, it will just work. This set of patches is just some preparation patches on the Orion watchdog driver, to later make it usable on Armada 370/XP. The patches enabling its usage on Armada 370/XP will follow.

But Ezequiel will confirm all that, we had a discussion together about this yesterday, so I guess what I said should be the plan, but it's better if Ezequiel confirms, obviously :)

Thanks!

Thomas

Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Hi Thomas

That was what i was thinking would happen. And then i started to wonder how well the kernel linker deals with multiple definitions of the same symbol. Dove and 370/XP can end up in the same kernel. So we need to have both orion-timer and the 370/XP timer in the same kernel, so we end up with the same symbol in the kernel twice...

Lets wait for Ezequiel to answer.

Andrew

Andrew, Thomas,

In the discussion about orion clocksource Russell was proposing a generic thread-safe write. That puts a single lock around all those writes. Of course, it will also blocked by totally unrelated thread-safe register accesses but should prevent us from having dozens of locks and solves the symbol issue.

Sebastian

Hi Sebastian,

Thanks for the insight! I'll try to dig this suggestion in the clocksource discussion. Do you have any plans for posting the clocksource soon? This way I can base this series on something that's in Jason's trees...

Thanks!

Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Hi Thomas, Andrew:

Thanks for looking at this!

Indeed that was one of the ideas. As Thomas said, this was just preparation work.

Yeah, well... I wasn't sure about using the same name, so another approach would be adding a new compatible to the driver and then make it use the appropriate function in the 370/XP clocksource driver (with a different name).

And, yet another approach, is what Sebastian just said, although I'm not sure I understood it :). In any case, we have already several solutions, which is why I'm not too worried about this particular issue.

On the other side, I'm much interested in knowing if you are OK with breaking the watchdog DT compatibility. If you NACK this, then I'll start preparing a different watchdog driver for 370/XP, since I don't want to extend a driver that is a bit dirty.

Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

Apparently there is some agreement that the bindings are still in flux and that they need to be for a bit longer in order to hammer out problems such as this.

Arnd and Olof both mentioned that something (a doc, and email?) is forthcoming about marking some bindings as stable. Whatever form that takes, this one wouldn't get the stable marking yet. ;-)

Oh, and one more nit. The work 'fix' triggers a whole bunch of "get on this right away, does it need to go to stable? Has anyone confirmed it? Which commit caused the regression? etc." Although I hate the word, I think 'refactoring' is much more appropriate description for this series.

thx,

Jason.

Yup, that's my understanding as well. But on the other side, I don't want to break possible users out there.

So, just to check, you say it's early enough to safely do such change?

In that case, I'll extend this patchset to include Armada 370/XP support and post it as soon as Sebastian's clocksource stuff gets in.

Oh, good observation. I wrote the cover letter at 8 PM, after ten long hours (*) of hacking and smashing this into something easy to review, and that's the best title I could come up with. I'll change it on v2.

Thanks,

(*) yes, I have another pair of eyes, in case these wear out.

Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

I agree, but living with a sub-standard binding is worse imho.

Well, it's not *safe*. But most consumers on mainline kernels are currently developer/hackers/distro maintainers. eg, folks who can handle it. It's better to do it now.

Cool.

thx,

Jason.