Armada 370/XP watchdog support

PATCH Superseded Ezequiel Garcia 2 participants 24 messages Jan 21, 2014 – Jan 27, 2014

Subsystems (6)

Device tree bindings Arm (32-bit) Arm sub-architectures Watchdog device drivers +2 more subsystems

lore

Share & tools

lore

Ezequiel Garcia

author

2014-01-21 13:25 UTC #0 lore raw

v3 cover Armada 370/XP watchdog support

Third patchset to extend Orion watchdog driver adding support for Armada 370/XP SoC. This consists on a small incremental series.

Please take a look at the previous version and the discussion raised:
http://www.spinics.net/lists/arm-kernel/msg302104.html

Changes from v2:

* Add proper error checking on clk_prepare_enable() and return PTR_ERR instead of ENODEV. Suggested by Fabio Estevam.

* After the usage of the atomic I/O and considering the watchdog core does its own serialization, the driver's spinlock was completely redundant and was removed. Also suggested by Fabio.

* Instead of making the driver dependent on PLAT_ORION, added a dependency to ARCH_MVEBU. This was proposed by Sebastian and Andrew, given we're working on PLAT_ORION removal.

This series is based on v3.13-rc8 and has been tested on:

* Marvell's Armada XP GP board

Ezequiel Garcia (15):
ARM: Introduce atomic MMIO modify
clocksource: orion: Use atomic access for shared registers
watchdog: orion: Use atomic access for shared registers
watchdog: orion: Handle IRQ
watchdog: orion: Make RSTOUT register a separate resource
watchdog: orion: Remove unneeded BRIDGE_CAUSE clear
watchdog: orion: Introduce an orion_watchdog device structure
watchdog: orion: Introduce per-compatible of_device_id data
watchdog: orion: Add per-compatible clock initialization
watchdog: orion: Add per-compatible watchdog start implementation
watchdog: orion: Add support for Armada 370 and Armada XP SoC
ARM: mvebu: Enable Armada 370/XP watchdog in the devicetree
ARM: kirkwood: Add RSTOUT 'reg' entry to devicetree
watchdog: orion: Enable the build on ARCH_MVEBU
ARM: mvebu: Enable watchdog support in defconfig

 .../devicetree/bindings/watchdog/marvel.txt        |   8 +-
 arch/arm/boot/dts/armada-370-xp.dtsi               |   4 +
 arch/arm/boot/dts/armada-370.dtsi                  |   5 +
 arch/arm/boot/dts/armada-xp.dtsi                   |   6 +
 arch/arm/boot/dts/kirkwood.dtsi                    |   2 +-
 arch/arm/configs/mvebu_defconfig                   |   2 +
 arch/arm/include/asm/io.h                          |   6 +
 arch/arm/kernel/io.c                               |  35 +++
 arch/arm/mach-dove/include/mach/bridge-regs.h      |   1 +
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h  |   1 +
 arch/arm/mach-mv78xx0/include/mach/bridge-regs.h   |   1 +
 arch/arm/mach-orion5x/include/mach/bridge-regs.h   |   1 +
 arch/arm/plat-orion/common.c                       |  10 +-
 drivers/clocksource/time-orion.c                   |  28 +-
 drivers/watchdog/Kconfig                           |   2 +-
 drivers/watchdog/orion_wdt.c                       | 320 ++++++++++++++++-----
 16 files changed, 329 insertions(+), 103 deletions(-)

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #1 lore raw

PATCH v3 1/15 ARM: Introduce atomic MMIO modify

Some SoC have MMIO regions that are shared across orthogonal subsystems. This commit implements a possible solution for the thread-safe access of such regions through a spinlock-protected API.

Concurrent access is protected with a single spinlock for the entire MMIO address space. While this protects shared-registers, it also serializes access to unrelated/unshared registers.

We add relaxed and non-relaxed variants, by using writel_relaxed and writel, respectively. The rationale for this is that some users may not require register write completion but only thread-safe access to a register.

Signed-off-by Ezequiel Garcia <[email protected]>

---
Submitted to ARM patch tracker:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=7930/1

 arch/arm/include/asm/io.h |  6 ++++++
 arch/arm/kernel/io.c      | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

arch/arm/include/asm/io.h +6 -0

index fbeb39c..8aa4cca 100644
@@ -38,6 +38,12 @@
 #define isa_bus_to_virt phys_to_virt
 
 /*
+ * Atomic MMIO-wide IO modify
+ */
+extern void atomic_io_modify(void __iomem *reg, u32 mask, u32 set);
+extern void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set);
+
+/*
  * Generic IO read/write.  These perform native-endian accesses.  Note
  * that some architectures will want to re-define __raw_{read,write}w.
  */

arch/arm/kernel/io.c +35 -0

index dcd5b4d..9203cf8 100644
@@ -1,6 +1,41 @@
 #include <linux/export.h>
 #include <linux/types.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
+
+static DEFINE_RAW_SPINLOCK(__io_lock);
+
+/*
+ * Generic atomic MMIO modify.
+ *
+ * Allows thread-safe access to registers shared by unrelated subsystems.
+ * The access is protected by a single MMIO-wide lock.
+ */
+void atomic_io_modify_relaxed(void __iomem *reg, u32 mask, u32 set)
+{
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&__io_lock, flags);
+	value = readl_relaxed(reg) & ~mask;
+	value |= (set & mask);
+	writel_relaxed(value, reg);
+	raw_spin_unlock_irqrestore(&__io_lock, flags);
+}
+EXPORT_SYMBOL(atomic_io_modify_relaxed);
+
+void atomic_io_modify(void __iomem *reg, u32 mask, u32 set)
+{
+	unsigned long flags;
+	u32 value;
+
+	raw_spin_lock_irqsave(&__io_lock, flags);
+	value = readl_relaxed(reg) & ~mask;
+	value |= (set & mask);
+	writel(value, reg);
+	raw_spin_unlock_irqrestore(&__io_lock, flags);
+}
+EXPORT_SYMBOL(atomic_io_modify);
 
 /*
  * Copy data from IO memory space to "real" memory space.

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #2 lore raw

PATCH v3 2/15 clocksource: orion: Use atomic access for shared registers

In reply to Ezequiel Garcia

Replace the driver-specific thread-safe shared register API by the recently introduced atomic_io_clear_set().

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/clocksource/time-orion.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

drivers/clocksource/time-orion.c +10 ~~-18~~

index 9c7f018..3f14e56 100644
@@ -35,20 +35,6 @@
 #define ORION_ONESHOT_MAX	0xfffffffe
 
 static void __iomem *timer_base;
-static DEFINE_SPINLOCK(timer_ctrl_lock);
-
-/*
- * Thread-safe access to TIMER_CTRL register
- * (shared with watchdog timer)
- */
-void orion_timer_ctrl_clrset(u32 clr, u32 set)
-{
-	spin_lock(&timer_ctrl_lock);
-	writel((readl(timer_base + TIMER_CTRL) & ~clr) | set,
-		timer_base + TIMER_CTRL);
-	spin_unlock(&timer_ctrl_lock);
-}
-EXPORT_SYMBOL(orion_timer_ctrl_clrset);
 
 /*
  * Free-running clocksource handling.

@@ -68,7 +54,8 @@ static int orion_clkevt_next_event(unsigned long delta,
 {
 	/* setup and enable one-shot timer */
 	writel(delta, timer_base + TIMER1_VAL);
-	orion_timer_ctrl_clrset(TIMER1_RELOAD_EN, TIMER1_EN);
+	atomic_io_modify(timer_base + TIMER_CTRL,
+		TIMER1_RELOAD_EN | TIMER1_EN, TIMER1_EN);
 
 	return 0;
 }

@@ -80,10 +67,13 @@ static void orion_clkevt_mode(enum clock_event_mode mode,
 		/* setup and enable periodic timer at 1/HZ intervals */
 		writel(ticks_per_jiffy - 1, timer_base + TIMER1_RELOAD);
 		writel(ticks_per_jiffy - 1, timer_base + TIMER1_VAL);
-		orion_timer_ctrl_clrset(0, TIMER1_RELOAD_EN | TIMER1_EN);
+		atomic_io_modify(timer_base + TIMER_CTRL,
+			TIMER1_RELOAD_EN | TIMER1_EN,
+			TIMER1_RELOAD_EN | TIMER1_EN);
 	} else {
 		/* disable timer */
-		orion_timer_ctrl_clrset(TIMER1_RELOAD_EN | TIMER1_EN, 0);
+		atomic_io_modify(timer_base + TIMER_CTRL,
+			TIMER1_RELOAD_EN | TIMER1_EN, 0);
 	}
 }

@@ -131,7 +121,9 @@ static void __init orion_timer_init(struct device_node *np)
 	/* setup timer0 as free-running clocksource */
 	writel(~0, timer_base + TIMER0_VAL);
 	writel(~0, timer_base + TIMER0_RELOAD);
-	orion_timer_ctrl_clrset(0, TIMER0_RELOAD_EN | TIMER0_EN);
+	atomic_io_modify(timer_base + TIMER_CTRL,
+		TIMER0_RELOAD_EN | TIMER0_EN,
+		TIMER0_RELOAD_EN | TIMER0_EN);
 	clocksource_mmio_init(timer_base + TIMER0_VAL, "orion_clocksource",
 			      clk_get_rate(clk), 300, 32,
 			      clocksource_mmio_readl_down);

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #3 lore raw

PATCH v3 3/15 watchdog: orion: Use atomic access for shared registers

In reply to Ezequiel Garcia

Since the timer control register is shared with the clocksource driver, use the recently introduced atomic_io_clear_set() to access such register. Given the watchdog core already provides serialization for all the watchdog ops, this commit allows to remove the spinlock entirely.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 42 +++++-------------------------------------
 1 file changed, 5 insertions(+), 37 deletions(-)

drivers/watchdog/orion_wdt.c +5 ~~-37~~

index f7722a4..0433ea8 100644
@@ -20,7 +20,6 @@
 #include <linux/watchdog.h>
 #include <linux/init.h>
 #include <linux/io.h>
-#include <linux/spinlock.h>
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/of.h>

@@ -46,25 +45,16 @@ static unsigned int wdt_max_duration;	/* (seconds) */
 static struct clk *clk;
 static unsigned int wdt_tclk;
 static void __iomem *wdt_reg;
-static DEFINE_SPINLOCK(wdt_lock);
 
 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);
-
-	spin_unlock(&wdt_lock);
 	return 0;
 }
 
 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 + WDT_VAL);

@@ -72,48 +62,26 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	writel(~WDT_INT_REQ, BRIDGE_CAUSE);
 
 	/* Enable watchdog timer */
-	reg = readl(wdt_reg + TIMER_CTRL);
-	reg |= WDT_EN;
-	writel(reg, wdt_reg + TIMER_CTRL);
+	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
 
 	/* Enable reset on watchdog */
-	reg = readl(RSTOUTn_MASK);
-	reg |= WDT_RESET_OUT_EN;
-	writel(reg, RSTOUTn_MASK);
-
-	spin_unlock(&wdt_lock);
+	atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
 	return 0;
 }
 
 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);
+	atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, 0);
 
 	/* Disable watchdog timer */
-	reg = readl(wdt_reg + TIMER_CTRL);
-	reg &= ~WDT_EN;
-	writel(reg, wdt_reg + TIMER_CTRL);
-
-	spin_unlock(&wdt_lock);
+	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0);
 	return 0;
 }
 
 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;
-	spin_unlock(&wdt_lock);
-
-	return time_left;
+	return readl(wdt_reg + WDT_VAL) / wdt_tclk;
 }
 
 static int orion_wdt_set_timeout(struct watchdog_device *wdt_dev,

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #4 lore raw

PATCH v3 4/15 watchdog: orion: Handle IRQ

In reply to Ezequiel Garcia

DT-enabled where an irqchip driver for the brigde interrupt controller is available can handle the watchdog IRQ properly. Therefore, we request the interruption and add a dummy handler that merely calls panic().

This is done in order to have an initial 'ack' of the interruption, which clears the watchdog state.

Furthermore, since some platforms don't have such IRQ, this commit makes the interruption specification optional.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 .../devicetree/bindings/watchdog/marvel.txt        |  2 ++
 drivers/watchdog/orion_wdt.c                       | 22 +++++++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/watchdog/marvel.txt +2 -0

index 5dc8d30..0731fbd 100644
@@ -7,6 +7,7 @@ Required Properties:
 
 Optional properties:
 
+- interrupts	: Contains the IRQ for watchdog expiration
 - timeout-sec	: Contains the watchdog timeout in seconds
 
 Example:

@@ -14,6 +15,7 @@ Example:
 	wdt at 20300 {
 		compatible = "marvell,orion-wdt";
 		reg = <0x20300 0x28>;
+		interrupts = <3>;
 		timeout-sec = <10>;
 		status = "okay";
 	};

drivers/watchdog/orion_wdt.c +21 -1

index 0433ea8..c8ab377 100644
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/clk.h>
 #include <linux/err.h>

@@ -111,10 +112,16 @@ static struct watchdog_device orion_wdt = {
 	.min_timeout = 1,
 };
 
+static irqreturn_t orion_wdt_irq(int irq, void *devid)
+{
+	panic("Watchdog Timeout");
+	return IRQ_HANDLED;
+}
+
 static int orion_wdt_probe(struct platform_device *pdev)
 {
 	struct resource *res;
-	int ret;
+	int ret, irq;
 
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {

@@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!wdt_reg)
 		return -ENOMEM;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
+		/*
+		 * Not all supported platforms specify an interruption for the
+		 * watchdog, so let's make it optional.
+		 */
+		ret = request_irq(irq, orion_wdt_irq, 0, pdev->name, NULL);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to request IRQ\n");
+			return ret;
+		}
+	}
+
 	wdt_max_duration = WDT_MAX_CYCLE_COUNT / wdt_tclk;
 
 	orion_wdt.timeout = wdt_max_duration;

Signature · 1 line

1.8.1.5

Guenter Roeck

maintainer

2014-01-21 14:31 UTC #5 lore raw

PATCH v3 4/15 watchdog: orion: Handle IRQ

Quoted 16 lines · Ezequiel Garcia

DT-enabled where an irqchip driver for the brigde interrupt controller is available can handle the watchdog IRQ properly. Therefore, we request the interruption and add a dummy handler that merely calls panic().
This is done in order to have an initial 'ack' of the interruption, which clears the watchdog state.
Furthermore, since some platforms don't have such IRQ, this commit makes the interruption specification optional.
Signed-off-by Ezequiel Garcia <[email protected]>
---
  .../devicetree/bindings/watchdog/marvel.txt        |  2 ++
  drivers/watchdog/orion_wdt.c                       | 22 +++++++++++++++++++++-
  2 files changed, 23 insertions(+), 1 deletion(-)

[ ... ]

Quoted 9 lines

  static int orion_wdt_probe(struct platform_device *pdev)
  {
  	struct resource *res;

drivers/watchdog/orion_wdt.c +1 -1

-	int ret;
+	int ret, irq;
 
  	clk = devm_clk_get(&pdev->dev, NULL);
  	if (IS_ERR(clk)) {

drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 142

Quoted 5 lines

drivers/watchdog/orion_wdt.c +2 -0

@@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev)
  	if (!wdt_reg)
  		return -ENOMEM;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {

0 is a valid interrupt number, and platform_get_irq returns an error code on errors. Should be >= 0.

Guenter

Ezequiel Garcia

author

2014-01-21 14:35 UTC #6 lore raw

PATCH v3 4/15 watchdog: orion: Handle IRQ

Quoted 32 lines · Guenter Roeck

Quoted 16 lines · Ezequiel Garcia
DT-enabled where an irqchip driver for the brigde interrupt controller is available can handle the watchdog IRQ properly. Therefore, we request the interruption and add a dummy handler that merely calls panic().
This is done in order to have an initial 'ack' of the interruption, which clears the watchdog state.
Furthermore, since some platforms don't have such IRQ, this commit makes the interruption specification optional.
Signed-off-by Ezequiel Garcia <[email protected]>
---
  .../devicetree/bindings/watchdog/marvel.txt        |  2 ++
  drivers/watchdog/orion_wdt.c                       | 22 +++++++++++++++++++++-
  2 files changed, 23 insertions(+), 1 deletion(-)
[ ... ]
Quoted 12 lines
  static int orion_wdt_probe(struct platform_device *pdev)
  {
  	struct resource *res;
drivers/watchdog/orion_wdt.c +1 -1
-	int ret;
+	int ret, irq;
 
  	clk = devm_clk_get(&pdev->dev, NULL);
  	if (IS_ERR(clk)) {
@@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev)
  	if (!wdt_reg)
  		return -ENOMEM;
 

Quoted 6 lines

drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 142
Quoted 2 lines
drivers/watchdog/orion_wdt.c +2 -0
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
0 is a valid interrupt number, and platform_get_irq returns an error code on errors. Should be >= 0.

Yes, indeed. I'll wait to see if there's any other feedback and then send a v4.

Thanks a lot!

Signature · 3 lines

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

Ezequiel Garcia

author

2014-01-27 06:30 UTC #7 lore raw

PATCH v3 4/15 watchdog: orion: Handle IRQ

In reply to Guenter Roeck

Hi Guenter,

On Tue, Jan 21, 2014 at 06:31:02AM -0800, Guenter Roeck wrote:
[..]

Quoted 10 lines

drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 142
Quoted 6 lines
drivers/watchdog/orion_wdt.c +2 -0
@@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev)
  	if (!wdt_reg)
  		return -ENOMEM;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
0 is a valid interrupt number, and platform_get_irq returns an error code on errors. Should be >= 0.

I'm revisiting this one. I believe this is not the hardware interrupt number, but the one mapped into virq space. So, 0 is not a valid interrupt number.

Right?

Signature · 7 lines

Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Guenter Roeck

maintainer

2014-01-27 06:42 UTC #8 lore raw

PATCH v3 4/15 watchdog: orion: Handle IRQ

Quoted 15 lines · Ezequiel Garcia

Hi Guenter,
On Tue, Jan 21, 2014 at 06:31:02AM -0800, Guenter Roeck wrote:
[..]
Quoted 10 lines
drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 142
Quoted 6 lines
drivers/watchdog/orion_wdt.c +2 -0
@@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev)
   	if (!wdt_reg)
   		return -ENOMEM;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
0 is a valid interrupt number, and platform_get_irq returns an error code on errors. Should be >= 0.

Quoted 6 lines

I'm revisiting this one. I believe this is not the hardware interrupt number, but the one mapped into virq space. So, 0 is not a valid interrupt number.
Right?

Hi,

If so, the entire interrupt numbering scheme appears broken. Conceptually it should not make a difference where the interrupt is coming from. If the virq system returns 0 for invalid (non-configured) interrupts, and non-configured 'real' interrupts are reported as -ENXIO, all bets are off. How would a driver know what to expect ? And how would one be expected to review such non-deterministic code ?

FWIW, platform_get_irq() does return -ENXIO for invalid interrupts. If there is an independent notion of "0 is an invalid interrupt", it is well hidden.

Anyway, if you think the driver should treat 0 as invalid interrupt, go ahead. Who am I to know. Just please don't use my Reviewed-by in this case.

Thanks,
Guenter

Ezequiel Garcia

author

2014-01-27 07:18 UTC #9 lore raw

PATCH v3 4/15 watchdog: orion: Handle IRQ

Quoted 23 lines · Guenter Roeck

Quoted 15 lines · Ezequiel Garcia
Hi Guenter,
On Tue, Jan 21, 2014 at 06:31:02AM -0800, Guenter Roeck wrote:
[..]
Quoted 10 lines
drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 142
Quoted 6 lines
drivers/watchdog/orion_wdt.c +2 -0
@@ -131,6 +138,19 @@ static int orion_wdt_probe(struct platform_device *pdev)
   	if (!wdt_reg)
   		return -ENOMEM;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq > 0) {
0 is a valid interrupt number, and platform_get_irq returns an error code on errors. Should be >= 0.
Quoted 6 lines
I'm revisiting this one. I believe this is not the hardware interrupt number, but the one mapped into virq space. So, 0 is not a valid interrupt number.
Right?

Quoted 7 lines

If so, the entire interrupt numbering scheme appears broken. Conceptually it should not make a difference where the interrupt is coming from. If the virq system returns 0 for invalid (non-configured) interrupts, and non-configured 'real' interrupts are reported as -ENXIO, all bets are off. How would a driver know what to expect ? And how would one be expected to review such non-deterministic code ?

I wouldn't know that much. I'm just pointing out that '0' doesn't seem to be a valid IRQ number in this context (i.e. virq space).

Quoted 3 lines

FWIW, platform_get_irq() does return -ENXIO for invalid interrupts. If there is an independent notion of "0 is an invalid interrupt", it is well hidden.

Yes, AFAICS platform_get_irq() returns a negative error or a positive irq number. I fail to see how it's able return '0' (of course, I can be wrong).

Quoted 3 lines

Anyway, if you think the driver should treat 0 as invalid interrupt, go ahead. Who am I to know. Just please don't use my Reviewed-by in this case.

Quite frankly not sure how to handle this best. A quick grep through the code doesn't help either: lots of drivers treat 0 as a valid interrupt and lots treat it as invalid. So I guess it doesn't really matter...

Can someone shed a light on this?

Signature · 7 lines

Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ezequiel Garcia

author

2014-01-21 13:26 UTC #10 lore raw

PATCH v3 5/15 watchdog: orion: Make RSTOUT register a separate resource

In reply to Ezequiel Garcia

In order to support other SoC, it's required to distinguish the 'control' timer register, from the 'rstout' register that enables system reset on watchdog expiration.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 Documentation/devicetree/bindings/watchdog/marvel.txt |  6 ++++--
 arch/arm/mach-dove/include/mach/bridge-regs.h         |  1 +
 arch/arm/mach-kirkwood/include/mach/bridge-regs.h     |  1 +
 arch/arm/mach-mv78xx0/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                          | 12 ++++++++++--
 7 files changed, 24 insertions(+), 8 deletions(-)

Documentation/devicetree/bindings/watchdog/marvel.txt +4 -2

index 0731fbd..1544fe9 100644
@@ -3,7 +3,9 @@
 Required Properties:
 
 - Compatibility : "marvell,orion-wdt"
-- reg		: Address of the timer registers
+- reg		: Should contain two entries: first one with the
+		  timer control address, second one with the
+		  rstout enable address.
 
 Optional properties:

@@ -14,7 +16,7 @@ Example:
 
 	wdt at 20300 {
 		compatible = "marvell,orion-wdt";
-		reg = <0x20300 0x28>;
+		reg = <0x20300 0x28>, <0x20108 0x4>;
 		interrupts = <3>;
 		timeout-sec = <10>;
 		status = "okay";

arch/arm/mach-dove/include/mach/bridge-regs.h +1 -0

index 5362df3..f4a5b34 100644
@@ -21,6 +21,7 @@
 #define  CPU_CTRL_PCIE1_LINK	0x00000008
 
 #define RSTOUTn_MASK		(BRIDGE_VIRT_BASE + 0x0108)
+#define RSTOUTn_MASK_PHYS	(BRIDGE_PHYS_BASE + 0x0108)
 #define  SOFT_RESET_OUT_EN	0x00000004
 
 #define SYSTEM_SOFT_RESET	(BRIDGE_VIRT_BASE + 0x010c)

arch/arm/mach-kirkwood/include/mach/bridge-regs.h +1 -0

index 8b9d1c9..60f6421 100644
@@ -21,6 +21,7 @@
 #define CPU_RESET		0x00000002
 
 #define RSTOUTn_MASK		(BRIDGE_VIRT_BASE + 0x0108)
+#define RSTOUTn_MASK_PHYS	(BRIDGE_PHYS_BASE + 0x0108)
 #define SOFT_RESET_OUT_EN	0x00000004
 
 #define SYSTEM_SOFT_RESET	(BRIDGE_VIRT_BASE + 0x010c)

arch/arm/mach-mv78xx0/include/mach/bridge-regs.h +1 -0

index 5f03484..e20d6da 100644
@@ -15,6 +15,7 @@
 #define L2_WRITETHROUGH		0x00020000
 
 #define RSTOUTn_MASK		(BRIDGE_VIRT_BASE + 0x0108)
+#define RSTOUTn_MASK_PHYS	(BRIDGE_PHYS_BASE + 0x0108)
 #define SOFT_RESET_OUT_EN	0x00000004
 
 #define SYSTEM_SOFT_RESET	(BRIDGE_VIRT_BASE + 0x010c)

arch/arm/mach-orion5x/include/mach/bridge-regs.h +1 -0

index f727d03..5766e3f 100644
@@ -18,6 +18,7 @@
 #define CPU_CTRL		(ORION5X_BRIDGE_VIRT_BASE + 0x104)
 
 #define RSTOUTn_MASK		(ORION5X_BRIDGE_VIRT_BASE + 0x108)
+#define RSTOUTn_MASK_PHYS	(ORION5X_BRIDGE_PHYS_BASE + 0x108)
 
 #define CPU_SOFT_RESET		(ORION5X_BRIDGE_VIRT_BASE + 0x10c)

arch/arm/plat-orion/common.c +6 -4

index c66d163..3375037 100644
@@ -594,14 +594,16 @@ void __init orion_spi_1_init(unsigned long mapbase)
 /*****************************************************************************
  * Watchdog
  ****************************************************************************/
-static struct resource orion_wdt_resource =
-		DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x28);
+static struct resource orion_wdt_resource[] = {
+		DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x04),
+		DEFINE_RES_MEM(RSTOUTn_MASK_PHYS, 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)

drivers/watchdog/orion_wdt.c +10 -2

index c8ab377..19e4adc 100644
@@ -46,6 +46,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 *wdt_rstout;
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {

@@ -66,14 +67,14 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
 
 	/* Enable reset on watchdog */
-	atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
+	atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
 	return 0;
 }
 
 static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 {
 	/* Disable reset on watchdog */
-	atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, 0);
+	atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, 0);
 
 	/* Disable watchdog timer */
 	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0);

@@ -138,6 +139,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;
+	wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!wdt_rstout)
+		return -ENOMEM;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq > 0) {
 		/*

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #11 lore raw

PATCH v3 6/15 watchdog: orion: Remove unneeded BRIDGE_CAUSE clear

In reply to Ezequiel Garcia

After adding the IRQ request, the BRIDGE_CAUSE bit should be cleared by the bridge interrupt controller. There's no longer a need to do it in the watchdog driver, so we can simply remove it.

Signed-off-by Ezequiel Garcia <[email protected]>

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

drivers/watchdog/orion_wdt.c +0 -5

index 19e4adc..7889309 100644
@@ -24,7 +24,6 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/of.h>
-#include <mach/bridge-regs.h>
 
 /*
  * Watchdog timer block registers.

@@ -38,7 +37,6 @@
 #define WDT_OK_TO_CLOSE		1
 
 #define WDT_RESET_OUT_EN	BIT(1)
-#define WDT_INT_REQ		BIT(3)
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */

@@ -60,9 +58,6 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	/* Set watchdog duration */
 	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
 
-	/* Clear watchdog timer interrupt */
-	writel(~WDT_INT_REQ, BRIDGE_CAUSE);
-
 	/* Enable watchdog timer */
 	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #12 lore raw

PATCH v3 7/15 watchdog: orion: Introduce an orion_watchdog device structure

In reply to Ezequiel Garcia

In order to prepare to support multiple compatible-strings, this commit adds a device structure to hold the driver's state.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 97 +++++++++++++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 37 deletions(-)

drivers/watchdog/orion_wdt.c +60 ~~-37~~

index 7889309..9a904be 100644
@@ -40,45 +40,56 @@
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
-static unsigned int wdt_max_duration;	/* (seconds) */
-static struct clk *clk;
-static unsigned int wdt_tclk;
-static void __iomem *wdt_reg;
-static void __iomem *wdt_rstout;
+
+struct orion_watchdog {
+	struct watchdog_device wdt;
+	void __iomem *reg;
+	void __iomem *rstout;
+	unsigned long clk_rate;
+	struct clk *clk;
+};
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 	/* Reload watchdog duration */
-	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
 	return 0;
 }
 
 static int orion_wdt_start(struct watchdog_device *wdt_dev)
 {
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
 	/* Set watchdog duration */
-	writel(wdt_tclk * wdt_dev->timeout, wdt_reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
 
 	/* Enable watchdog timer */
-	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN);
+	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, WDT_EN);
 
 	/* Enable reset on watchdog */
-	atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
+	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
+
 	return 0;
 }
 
 static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 {
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
 	/* Disable reset on watchdog */
-	atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, 0);
+	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, 0);
 
 	/* Disable watchdog timer */
-	atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0);
+	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, 0);
+
 	return 0;
 }
 
 static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 {
-	return readl(wdt_reg + WDT_VAL) / wdt_tclk;
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+	return readl(dev->reg + WDT_VAL) / dev->clk_rate;
 }
 
 static int orion_wdt_set_timeout(struct watchdog_device *wdt_dev,

@@ -102,12 +113,6 @@ static const struct watchdog_ops orion_wdt_ops = {
 	.get_timeleft = orion_wdt_get_timeleft,
 };
 
-static struct watchdog_device orion_wdt = {
-	.info = &orion_wdt_info,
-	.ops = &orion_wdt_ops,
-	.min_timeout = 1,
-};
-
 static irqreturn_t orion_wdt_irq(int irq, void *devid)
 {
 	panic("Watchdog Timeout");

@@ -116,29 +121,41 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
 
 static int orion_wdt_probe(struct platform_device *pdev)
 {
+	struct orion_watchdog *dev;
+	unsigned int wdt_max_duration;	/* (seconds) */
 	struct resource *res;
 	int ret, irq;
 
-	clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(clk)) {
+	dev = devm_kzalloc(&pdev->dev, sizeof(struct orion_watchdog),
+			   GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->wdt.info = &orion_wdt_info;
+	dev->wdt.ops = &orion_wdt_ops;
+	dev->wdt.min_timeout = 1;
+
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk)) {
 		dev_err(&pdev->dev, "Orion Watchdog missing clock\n");
 		return -ENODEV;
 	}
-	clk_prepare_enable(clk);
-	wdt_tclk = clk_get_rate(clk);
+	clk_prepare_enable(dev->clk);
+	dev->clk_rate = clk_get_rate(dev->clk);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENODEV;
-	wdt_reg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
-	if (!wdt_reg)
+	dev->reg = devm_ioremap(&pdev->dev, res->start,
+			       resource_size(res));
+	if (!dev->reg)
 		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 	if (!res)
 		return -ENODEV;
-	wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res));
-	if (!wdt_rstout)
+	dev->rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!dev->rstout)
 		return -ENOMEM;
 
 	irq = platform_get_irq(pdev, 0);

@@ -154,34 +171,40 @@ static int orion_wdt_probe(struct platform_device *pdev)
 		}
 	}
 
-	wdt_max_duration = WDT_MAX_CYCLE_COUNT / wdt_tclk;
+	wdt_max_duration = WDT_MAX_CYCLE_COUNT / dev->clk_rate;
 
-	orion_wdt.timeout = wdt_max_duration;
-	orion_wdt.max_timeout = wdt_max_duration;
-	watchdog_init_timeout(&orion_wdt, heartbeat, &pdev->dev);
+	dev->wdt.timeout = wdt_max_duration;
+	dev->wdt.max_timeout = wdt_max_duration;
+	watchdog_init_timeout(&dev->wdt, heartbeat, &pdev->dev);
 
-	watchdog_set_nowayout(&orion_wdt, nowayout);
-	ret = watchdog_register_device(&orion_wdt);
+	platform_set_drvdata(pdev, &dev->wdt);
+	watchdog_set_drvdata(&dev->wdt, dev);
+	watchdog_set_nowayout(&dev->wdt, nowayout);
+	ret = watchdog_register_device(&dev->wdt);
 	if (ret) {
-		clk_disable_unprepare(clk);
+		clk_disable_unprepare(dev->clk);
 		return ret;
 	}
 
 	pr_info("Initial timeout %d sec%s\n",
-		orion_wdt.timeout, nowayout ? ", nowayout" : "");
+		dev->wdt.timeout, nowayout ? ", nowayout" : "");
 	return 0;
 }
 
 static int orion_wdt_remove(struct platform_device *pdev)
 {
-	watchdog_unregister_device(&orion_wdt);
-	clk_disable_unprepare(clk);
+	struct watchdog_device *wdt_dev = platform_get_drvdata(pdev);
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	watchdog_unregister_device(wdt_dev);
+	clk_disable_unprepare(dev->clk);
 	return 0;
 }
 
 static void orion_wdt_shutdown(struct platform_device *pdev)
 {
-	orion_wdt_stop(&orion_wdt);
+	struct watchdog_device *wdt_dev = platform_get_drvdata(pdev);
+	orion_wdt_stop(wdt_dev);
 }
 
 static const struct of_device_id orion_wdt_of_match_table[] = {

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #13 lore raw

PATCH v3 8/15 watchdog: orion: Introduce per-compatible of_device_id data

In reply to Ezequiel Garcia

This commit adds an orion_watchdog_data structure to holda compatible-data information. This allows to remove the driver-wide definition and to future add support for multiple compatible-strings.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 62 ++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 16 deletions(-)

drivers/watchdog/orion_wdt.c +46 ~~-16~~

index 9a904be..5108496 100644
@@ -24,36 +24,47 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 
 /*
  * Watchdog timer block registers.
  */
 #define TIMER_CTRL		0x0000
-#define WDT_EN			0x0010
-#define WDT_VAL			0x0024
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0
 #define WDT_OK_TO_CLOSE		1
 
-#define WDT_RESET_OUT_EN	BIT(1)
+#define WDT_A370_RATIO_MASK(v)	((v) << 16)
+#define WDT_A370_RATIO_SHIFT	5
+#define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
+
+#define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
 
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
 
+struct orion_watchdog_data {
+	int wdt_counter_offset;
+	int wdt_enable_bit;
+	int rstout_enable_bit;
+};
+
 struct orion_watchdog {
 	struct watchdog_device wdt;
 	void __iomem *reg;
 	void __iomem *rstout;
 	unsigned long clk_rate;
 	struct clk *clk;
+	struct orion_watchdog_data *data;
 };
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 	/* Reload watchdog duration */
-	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
 	return 0;
 }

@@ -62,13 +73,16 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 
 	/* Set watchdog duration */
-	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
 
 	/* Enable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, WDT_EN);
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
+						dev->data->wdt_enable_bit);
 
 	/* Enable reset on watchdog */
-	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
+				      dev->data->rstout_enable_bit);
 
 	return 0;
 }

@@ -78,10 +92,10 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 
 	/* Disable reset on watchdog */
-	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, 0);
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit, 0);
 
 	/* Disable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, 0);
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit, 0);
 
 	return 0;
 }

@@ -89,7 +103,7 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
-	return readl(dev->reg + WDT_VAL) / dev->clk_rate;
+	return readl(dev->reg + dev->data->wdt_counter_offset) / dev->clk_rate;
 }
 
 static int orion_wdt_set_timeout(struct watchdog_device *wdt_dev,

@@ -119,9 +133,25 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
+static const struct orion_watchdog_data orion_data = {
+	.rstout_enable_bit = BIT(1),
+	.wdt_enable_bit = BIT(4),
+	.wdt_counter_offset = 0x24,
+};
+
+static const struct of_device_id orion_wdt_of_match_table[] = {
+	{
+		.compatible = "marvell,orion-wdt",
+		.data = &orion_data,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);
+
 static int orion_wdt_probe(struct platform_device *pdev)
 {
 	struct orion_watchdog *dev;
+	const struct of_device_id *match;
 	unsigned int wdt_max_duration;	/* (seconds) */
 	struct resource *res;
 	int ret, irq;

@@ -131,9 +161,15 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!dev)
 		return -ENOMEM;
 
+	match = of_match_device(orion_wdt_of_match_table, &pdev->dev);
+	if (!match)
+		/* Default legacy match */
+		match = &orion_wdt_of_match_table[0];
+
 	dev->wdt.info = &orion_wdt_info;
 	dev->wdt.ops = &orion_wdt_ops;
 	dev->wdt.min_timeout = 1;
+	dev->data = (struct orion_watchdog_data *)match->data;
 
 	dev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dev->clk)) {

@@ -207,12 +243,6 @@ static void orion_wdt_shutdown(struct platform_device *pdev)
 	orion_wdt_stop(wdt_dev);
 }
 
-static const struct of_device_id orion_wdt_of_match_table[] = {
-	{ .compatible = "marvell,orion-wdt", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);
-
 static struct platform_driver orion_wdt_driver = {
 	.probe		= orion_wdt_probe,
 	.remove		= orion_wdt_remove,

Signature · 1 line

1.8.1.5

Guenter Roeck

maintainer

2014-01-21 16:35 UTC #14 lore raw

PATCH v3 8/15 watchdog: orion: Introduce per-compatible of_device_id data

Quoted 1 line · Ezequiel Garcia

This commit adds an orion_watchdog_data structure to holda compatible-data

s/holda/hold/

Quoted 2 lines

information. This allows to remove the driver-wide definition and to future add support for multiple compatible-strings.

Maybe "and to be able to add support for multiple compatible-strings in the future" ?

Quoted 17 lines

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 62 ++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 16 deletions(-)

drivers/watchdog/orion_wdt.c +1 -0

index 9a904be..5108496 100644
@@ -24,36 +24,47 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 
 /*

drivers/watchdog/orion_wdt.c line 37

Quoted 10 lines

drivers/watchdog/orion_wdt.c +0 -2

  * Watchdog timer block registers.
  */
 #define TIMER_CTRL		0x0000
-#define WDT_EN			0x0010
-#define WDT_VAL			0x0024
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0
 #define WDT_OK_TO_CLOSE		1

While looking at the new defines below, I noticed that WDT_IN_USE and WDT_OK_TO_CLOSE are not used (anymore ?) and thus should be removed (separate patch, though).

drivers/watchdog/orion_wdt.c line 43

Quoted 7 lines

drivers/watchdog/orion_wdt.c +5 -1

-#define WDT_RESET_OUT_EN	BIT(1)
+#define WDT_A370_RATIO_MASK(v)	((v) << 16)
+#define WDT_A370_RATIO_SHIFT	5
+#define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
+
+#define WDT_AXP_FIXED_ENABLE_BIT BIT(10)

Those new defines are not used. They should be introduced if and when used.

Quoted 99 lines

drivers/watchdog/orion_wdt.c +34 -7

 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
 
+struct orion_watchdog_data {
+	int wdt_counter_offset;
+	int wdt_enable_bit;
+	int rstout_enable_bit;
+};
+
 struct orion_watchdog {
 	struct watchdog_device wdt;
 	void __iomem *reg;
 	void __iomem *rstout;
 	unsigned long clk_rate;
 	struct clk *clk;
+	struct orion_watchdog_data *data;
 };
 
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 	/* Reload watchdog duration */
-	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
 	return 0;
 }

@@ -62,13 +73,16 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 
 	/* Set watchdog duration */
-	writel(dev->clk_rate * wdt_dev->timeout, dev->reg + WDT_VAL);
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
 
 	/* Enable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, WDT_EN);
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
+						dev->data->wdt_enable_bit);
 
 	/* Enable reset on watchdog */
-	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN);
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
+				      dev->data->rstout_enable_bit);
 
 	return 0;
 }

@@ -78,10 +92,10 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
 
 	/* Disable reset on watchdog */
-	atomic_io_modify(dev->rstout, WDT_RESET_OUT_EN, 0);
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit, 0);
 
 	/* Disable watchdog timer */
-	atomic_io_modify(dev->reg + TIMER_CTRL, WDT_EN, 0);
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit, 0);
 
 	return 0;
 }

@@ -89,7 +103,7 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 static unsigned int orion_wdt_get_timeleft(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
-	return readl(dev->reg + WDT_VAL) / dev->clk_rate;
+	return readl(dev->reg + dev->data->wdt_counter_offset) / dev->clk_rate;
 }
 
 static int orion_wdt_set_timeout(struct watchdog_device *wdt_dev,

@@ -119,9 +133,25 @@ static irqreturn_t orion_wdt_irq(int irq, void *devid)
 	return IRQ_HANDLED;
 }
 
+static const struct orion_watchdog_data orion_data = {
+	.rstout_enable_bit = BIT(1),
+	.wdt_enable_bit = BIT(4),
+	.wdt_counter_offset = 0x24,
+};
+
+static const struct of_device_id orion_wdt_of_match_table[] = {
+	{
+		.compatible = "marvell,orion-wdt",
+		.data = &orion_data,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);
+
 static int orion_wdt_probe(struct platform_device *pdev)
 {
 	struct orion_watchdog *dev;
+	const struct of_device_id *match;
 	unsigned int wdt_max_duration;	/* (seconds) */
 	struct resource *res;
 	int ret, irq;

@@ -131,9 +161,15 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!dev)
 		return -ENOMEM;

drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 172

Quoted 10 lines

drivers/watchdog/orion_wdt.c +6 -0

 
+	match = of_match_device(orion_wdt_of_match_table, &pdev->dev);
+	if (!match)
+		/* Default legacy match */
+		match = &orion_wdt_of_match_table[0];
+
 	dev->wdt.info = &orion_wdt_info;
 	dev->wdt.ops = &orion_wdt_ops;
 	dev->wdt.min_timeout = 1;
+	dev->data = (struct orion_watchdog_data *)match->data;

match->data is a void *, so the typecast should not be needed here. You might want to declare 'data' to be of type
'const struct orion_watchdog_data *' because otherwise you loose the 'const' attribute of match->data (which may be the reason for the typecast ?).

Guenter

Ezequiel Garcia

author

2014-01-22 17:06 UTC #15 lore raw

PATCH v3 8/15 watchdog: orion: Introduce per-compatible of_device_id data

Quoted 5 lines · Guenter Roeck

Quoted 1 line · Ezequiel Garcia
This commit adds an orion_watchdog_data structure to holda compatible-data
s/holda/hold/

Yup.

Quoted 6 lines

Quoted 2 lines
information. This allows to remove the driver-wide definition and to future add support for multiple compatible-strings.
Maybe "and to be able to add support for multiple compatible-strings in the future" ?

Yes, sounds better.

[..]

Quoted 9 lines

Quoted 5 lines
#define WDT_MAX_CYCLE_COUNT 0xffffffff
#define WDT_IN_USE 0
#define WDT_OK_TO_CLOSE 1
While looking at the new defines below, I noticed that WDT_IN_USE and WDT_OK_TO_CLOSE are not used (anymore ?) and thus should be removed (separate patch, though).

OK, let's clean this.

Quoted 9 lines

drivers/watchdog/orion_wdt.c line 42
Quoted 7 lines
drivers/watchdog/orion_wdt.c +5 -1
-#define WDT_RESET_OUT_EN	BIT(1)
+#define WDT_A370_RATIO_MASK(v)	((v) << 16)
+#define WDT_A370_RATIO_SHIFT	5
+#define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
+
+#define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
Those new defines are not used. They should be introduced if and when used.

Hm... maybe after dozens and dozens of rebases these macros sneaked here?

[..]

Quoted 16 lines

drivers/watchdog/orion_wdt.c static int orion_wdt_probe(struct platform_device *pdev) · line 172
Quoted 10 lines
drivers/watchdog/orion_wdt.c +6 -0
+	match = of_match_device(orion_wdt_of_match_table, &pdev->dev);
+	if (!match)
+		/* Default legacy match */
+		match = &orion_wdt_of_match_table[0];
+
 	dev->wdt.info = &orion_wdt_info;
 	dev->wdt.ops = &orion_wdt_ops;
 	dev->wdt.min_timeout = 1;
+	dev->data = (struct orion_watchdog_data *)match->data;
match->data is a void *, so the typecast should not be needed here. You might want to declare 'data' to be of type
'const struct orion_watchdog_data *' because otherwise you loose the 'const' attribute of match->data (which may be the reason for the typecast ?).

Done.

Thanks a lot for reviewing!

Signature · 7 lines

Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ezequiel Garcia

author

2014-01-21 13:26 UTC #16 lore raw

PATCH v3 9/15 watchdog: orion: Add per-compatible clock initialization

In reply to Ezequiel Garcia

Following the introduction of the compatible-data field, it's now possible to further abstract the clock initialization. This will allow to support SoC with a different clock setup.

While we're at it, let's fix the clock error handling by returning PTR_ERR and checking clk_prepare_enable().

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

drivers/watchdog/orion_wdt.c +27 -8

index 5108496..b8d0f87 100644
@@ -44,10 +44,14 @@
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */
 
+struct orion_watchdog;
+
 struct orion_watchdog_data {
 	int wdt_counter_offset;
 	int wdt_enable_bit;
 	int rstout_enable_bit;
+	int (*clock_init) (struct platform_device *,
+			   struct orion_watchdog *);
 };
 
 struct orion_watchdog {

@@ -59,6 +63,22 @@ struct orion_watchdog {
 	struct orion_watchdog_data *data;
 };
 
+static int orion_wdt_clock_init(struct platform_device *pdev,
+				struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+	ret = clk_prepare_enable(dev->clk);
+	if (ret)
+		return ret;
+
+	dev->clk_rate = clk_get_rate(dev->clk);
+	return 0;
+}
+
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);

@@ -137,6 +157,7 @@ static const struct orion_watchdog_data orion_data = {
 	.rstout_enable_bit = BIT(1),
 	.wdt_enable_bit = BIT(4),
 	.wdt_counter_offset = 0x24,
+	.clock_init = orion_wdt_clock_init,
 };
 
 static const struct of_device_id orion_wdt_of_match_table[] = {

@@ -171,14 +192,6 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	dev->wdt.min_timeout = 1;
 	dev->data = (struct orion_watchdog_data *)match->data;
 
-	dev->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(dev->clk)) {
-		dev_err(&pdev->dev, "Orion Watchdog missing clock\n");
-		return -ENODEV;
-	}
-	clk_prepare_enable(dev->clk);
-	dev->clk_rate = clk_get_rate(dev->clk);
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENODEV;

@@ -194,6 +207,12 @@ static int orion_wdt_probe(struct platform_device *pdev)
 	if (!dev->rstout)
 		return -ENOMEM;
 
+	ret = dev->data->clock_init(pdev, dev);
+	if (ret) {
+		dev_err(&pdev->dev, "cannot initialize clock\n");
+		return ret;
+	}
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq > 0) {
 		/*

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #17 lore raw

PATCH v3 10/15 watchdog: orion: Add per-compatible watchdog start implementation

In reply to Ezequiel Garcia

To handle differences between SoCs this commit adds per-compatible string start() function for the watchdog kick-off. This is preparation work and makes no functionality changes to the current driver.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/orion_wdt.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

drivers/watchdog/orion_wdt.c +11 -1

index b8d0f87..c2beb9f 100644
@@ -52,6 +52,7 @@ struct orion_watchdog_data {
 	int rstout_enable_bit;
 	int (*clock_init) (struct platform_device *,
 			   struct orion_watchdog *);
+	int (*start) (struct watchdog_device *);
 };
 
 struct orion_watchdog {

@@ -88,7 +89,7 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
-static int orion_wdt_start(struct watchdog_device *wdt_dev)
+static int orion_start(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);

@@ -107,6 +108,14 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static int orion_wdt_start(struct watchdog_device *wdt_dev)
+{
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	/* There are some per-SoC quirks to handle */
+	return dev->data->start(wdt_dev);
+}
+
 static int orion_wdt_stop(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);

@@ -158,6 +167,7 @@ static const struct orion_watchdog_data orion_data = {
 	.wdt_enable_bit = BIT(4),
 	.wdt_counter_offset = 0x24,
 	.clock_init = orion_wdt_clock_init,
+	.start = orion_start,
 };
 
 static const struct of_device_id orion_wdt_of_match_table[] = {

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #18 lore raw

PATCH v3 11/15 watchdog: orion: Add support for Armada 370 and Armada XP SoC

In reply to Ezequiel Garcia

Using the added infrastructure for handling SoC differences, this commit adds support for the watchdog controller available in Armada 370 and Armada XP SoCs.

Signed-off-by Ezequiel Garcia <[email protected]>

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

drivers/watchdog/orion_wdt.c +93 -0

index c2beb9f..a5e157c 100644
@@ -30,6 +30,7 @@
  * Watchdog timer block registers.
  */
 #define TIMER_CTRL		0x0000
+#define TIMER_A370_STATUS	0x04
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0

@@ -41,6 +42,12 @@
 
 #define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
 
+#define WDT_A370_RATIO_MASK(v)	((v) << 16)
+#define WDT_A370_RATIO_SHIFT	5
+#define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
+
+#define WDT_A370_EXPIRED	BIT(31)
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */

@@ -80,6 +87,48 @@ static int orion_wdt_clock_init(struct platform_device *pdev,
 	return 0;
 }
 
+static int armada370_wdt_clock_init(struct platform_device *pdev,
+				    struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+	ret = clk_prepare_enable(dev->clk);
+	if (ret)
+		return ret;
+
+	/* Setup watchdog input clock */
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
+
+	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
+	return 0;
+}
+
+static int armadaxp_wdt_clock_init(struct platform_device *pdev,
+				   struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+	ret = clk_prepare_enable(dev->clk);
+	if (ret)
+		return ret;
+
+	/* Enable the fixed watchdog clock input */
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+		WDT_AXP_FIXED_ENABLE_BIT,
+		WDT_AXP_FIXED_ENABLE_BIT);
+
+	dev->clk_rate = clk_get_rate(dev->clk);
+	return 0;
+}
+
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);

@@ -89,6 +138,26 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static int armada370_start(struct watchdog_device *wdt_dev)
+{
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	/* Set watchdog duration */
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
+
+	/* Clear the watchdog expiration bit */
+	atomic_io_modify(dev->reg + TIMER_A370_STATUS, WDT_A370_EXPIRED, 0);
+
+	/* Enable watchdog timer */
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
+						dev->data->wdt_enable_bit);
+
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
+				      dev->data->rstout_enable_bit);
+	return 0;
+}
+
 static int orion_start(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);

@@ -170,11 +239,35 @@ static const struct orion_watchdog_data orion_data = {
 	.start = orion_start,
 };
 
+static const struct orion_watchdog_data armada370_data = {
+	.rstout_enable_bit = BIT(8),
+	.wdt_enable_bit = BIT(8),
+	.wdt_counter_offset = 0x34,
+	.clock_init = armada370_wdt_clock_init,
+	.start = armada370_start,
+};
+
+static const struct orion_watchdog_data armadaxp_data = {
+	.rstout_enable_bit = BIT(8),
+	.wdt_enable_bit = BIT(8),
+	.wdt_counter_offset = 0x34,
+	.clock_init = armadaxp_wdt_clock_init,
+	.start = armada370_start,
+};
+
 static const struct of_device_id orion_wdt_of_match_table[] = {
 	{
 		.compatible = "marvell,orion-wdt",
 		.data = &orion_data,
 	},
+	{
+		.compatible = "marvell,armada-370-wdt",
+		.data = &armada370_data,
+	},
+	{
+		.compatible = "marvell,armada-xp-wdt",
+		.data = &armadaxp_data,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);

Signature · 1 line

1.8.1.5

Guenter Roeck

maintainer

2014-01-21 16:41 UTC #19 lore raw

PATCH v3 11/15 watchdog: orion: Add support for Armada 370 and Armada XP SoC

Quoted 22 lines · Ezequiel Garcia

Using the added infrastructure for handling SoC differences, this commit adds support for the watchdog controller available in Armada 370 and Armada XP SoCs.
Signed-off-by Ezequiel Garcia <[email protected]>
---
 drivers/watchdog/orion_wdt.c | 93 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
drivers/watchdog/orion_wdt.c +1 -0
index c2beb9f..a5e157c 100644
@@ -30,6 +30,7 @@
  * Watchdog timer block registers.
  */
 #define TIMER_CTRL		0x0000
+#define TIMER_A370_STATUS	0x04
 
 #define WDT_MAX_CYCLE_COUNT	0xffffffff
 #define WDT_IN_USE		0

drivers/watchdog/orion_wdt.c line 48

Quoted 7 lines

drivers/watchdog/orion_wdt.c +4 -0

@@ -41,6 +42,12 @@
 
 #define WDT_AXP_FIXED_ENABLE_BIT BIT(10)
 
+#define WDT_A370_RATIO_MASK(v)	((v) << 16)
+#define WDT_A370_RATIO_SHIFT	5
+#define WDT_A370_RATIO		(1 << WDT_A370_RATIO_SHIFT)
+

Are those introduced twice ? I seem to recall seeing those defines in patch 8.

Quoted 36 lines

drivers/watchdog/orion_wdt.c +29 -0

+#define WDT_A370_EXPIRED	BIT(31)
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 static int heartbeat = -1;		/* module parameter (seconds) */

@@ -80,6 +87,48 @@ static int orion_wdt_clock_init(struct platform_device *pdev,
 	return 0;
 }
 
+static int armada370_wdt_clock_init(struct platform_device *pdev,
+				    struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+	ret = clk_prepare_enable(dev->clk);
+	if (ret)
+		return ret;
+
+	/* Setup watchdog input clock */
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT),
+			WDT_A370_RATIO_MASK(WDT_A370_RATIO_SHIFT));
+
+	dev->clk_rate = clk_get_rate(dev->clk) / WDT_A370_RATIO;
+	return 0;
+}
+
+static int armadaxp_wdt_clock_init(struct platform_device *pdev,
+				   struct orion_watchdog *dev)
+{
+	int ret;
+
+	dev->clk = of_clk_get_by_name(pdev->dev.of_node, "fixed");

drivers/watchdog/orion_wdt.c static int orion_wdt_clock_init(struct platform_device *pdev, · line 126

Quoted 10 lines

drivers/watchdog/orion_wdt.c +10 -0

+	if (IS_ERR(dev->clk))
+		return PTR_ERR(dev->clk);
+	ret = clk_prepare_enable(dev->clk);
+	if (ret)
+		return ret;
+
+	/* Enable the fixed watchdog clock input */
+	atomic_io_modify(dev->reg + TIMER_CTRL,
+		WDT_AXP_FIXED_ENABLE_BIT,
+		WDT_AXP_FIXED_ENABLE_BIT);

Who am I to complain, but your continuation line indentation isn't really consistent ;-).

Quoted 79 lines

drivers/watchdog/orion_wdt.c +49 -0

+
+	dev->clk_rate = clk_get_rate(dev->clk);
+	return 0;
+}
+
 static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);

@@ -89,6 +138,26 @@ static int orion_wdt_ping(struct watchdog_device *wdt_dev)
 	return 0;
 }
 
+static int armada370_start(struct watchdog_device *wdt_dev)
+{
+	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);
+
+	/* Set watchdog duration */
+	writel(dev->clk_rate * wdt_dev->timeout,
+	       dev->reg + dev->data->wdt_counter_offset);
+
+	/* Clear the watchdog expiration bit */
+	atomic_io_modify(dev->reg + TIMER_A370_STATUS, WDT_A370_EXPIRED, 0);
+
+	/* Enable watchdog timer */
+	atomic_io_modify(dev->reg + TIMER_CTRL, dev->data->wdt_enable_bit,
+						dev->data->wdt_enable_bit);
+
+	atomic_io_modify(dev->rstout, dev->data->rstout_enable_bit,
+				      dev->data->rstout_enable_bit);
+	return 0;
+}
+
 static int orion_start(struct watchdog_device *wdt_dev)
 {
 	struct orion_watchdog *dev = watchdog_get_drvdata(wdt_dev);

@@ -170,11 +239,35 @@ static const struct orion_watchdog_data orion_data = {
 	.start = orion_start,
 };
 
+static const struct orion_watchdog_data armada370_data = {
+	.rstout_enable_bit = BIT(8),
+	.wdt_enable_bit = BIT(8),
+	.wdt_counter_offset = 0x34,
+	.clock_init = armada370_wdt_clock_init,
+	.start = armada370_start,
+};
+
+static const struct orion_watchdog_data armadaxp_data = {
+	.rstout_enable_bit = BIT(8),
+	.wdt_enable_bit = BIT(8),
+	.wdt_counter_offset = 0x34,
+	.clock_init = armadaxp_wdt_clock_init,
+	.start = armada370_start,
+};
+
 static const struct of_device_id orion_wdt_of_match_table[] = {
 	{
 		.compatible = "marvell,orion-wdt",
 		.data = &orion_data,
 	},
+	{
+		.compatible = "marvell,armada-370-wdt",
+		.data = &armada370_data,
+	},
+	{
+		.compatible = "marvell,armada-xp-wdt",
+		.data = &armadaxp_data,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, orion_wdt_of_match_table);

Signature · 6 lines

1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ezequiel Garcia

author

2014-01-21 13:26 UTC #20 lore raw

PATCH v3 12/15 ARM: mvebu: Enable Armada 370/XP watchdog in the devicetree

In reply to Ezequiel Garcia

Add the DT nodes to enable watchdog support available in Armada 370 and Armada XP SoCs.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/boot/dts/armada-370-xp.dtsi | 4 ++++
 arch/arm/boot/dts/armada-370.dtsi    | 5 +++++
 arch/arm/boot/dts/armada-xp.dtsi     | 6 ++++++
 3 files changed, 15 insertions(+)

arch/arm/boot/dts/armada-370-xp.dtsi +4 -0

index 7f10f62..96e0389 100644
@@ -151,6 +151,10 @@
 				interrupts = <37>, <38>, <39>, <40>, <5>, <6>;
 			};
 
+			watchdog at 20300 {
+				reg = <0x20300 0x34>, <0x20704 0x4>;
+			};
+
 			sata at a0000 {
 				compatible = "marvell,orion-sata";
 				reg = <0xa0000 0x5000>;

arch/arm/boot/dts/armada-370.dtsi +5 -0

index 7a4b82e..aebed9e 100644
@@ -168,6 +168,11 @@
 				clocks = <&coreclk 2>;
 			};
 
+			watchdog at 20300 {
+				compatible = "marvell,armada-370-wdt";
+				clocks = <&coreclk 2>;
+			};
+
 			coreclk: mvebu-sar at 18230 {
 				compatible = "marvell,armada-370-core-clock";
 				reg = <0x18230 0x08>;

arch/arm/boot/dts/armada-xp.dtsi +6 -0

index 281c644..8c6c06c 100644
@@ -74,6 +74,12 @@
 				clock-names = "nbclk", "fixed";
 			};
 
+			watchdog at 20300 {
+				compatible = "marvell,armada-xp-wdt";
+				clocks = <&coreclk 2>, <&refclk>;
+				clock-names = "nbclk", "fixed";
+			};
+
 			coreclk: mvebu-sar at 18230 {
 				compatible = "marvell,armada-xp-core-clock";
 				reg = <0x18230 0x08>;

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #21 lore raw

PATCH v3 13/15 ARM: kirkwood: Add RSTOUT 'reg' entry to devicetree

In reply to Ezequiel Garcia

In order to support multiplatform builds the watchdog devicetree binding was modified and now the 'reg' property is specified to need two entries. This commit adds the second entry as-per the new specification.

Signed-off-by Ezequiel Garcia <[email protected]>

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

arch/arm/boot/dts/kirkwood.dtsi +1 -1

index 8b73c80..80a56b0 100644
@@ -165,7 +165,7 @@
 
 		wdt: watchdog-timer at 20300 {
 			compatible = "marvell,orion-wdt";
-			reg = <0x20300 0x28>;
+			reg = <0x20300 0x28>, <0x20108 0x4>;
 			interrupt-parent = <&bridge_intc>;
 			interrupts = <3>;
 			clocks = <&gate_clk 7>;

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #22 lore raw

PATCH v3 14/15 watchdog: orion: Enable the build on ARCH_MVEBU

In reply to Ezequiel Garcia

After adding support for Armada 370/XP SoC let's enable the build on these platforms.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/watchdog/Kconfig +1 -1

index 5be6e91..8b79012 100644
@@ -282,7 +282,7 @@ config DAVINCI_WATCHDOG
 
 config ORION_WATCHDOG
 	tristate "Orion watchdog"
-	depends on ARCH_ORION5X || ARCH_KIRKWOOD || ARCH_DOVE
+	depends on ARCH_ORION5X || ARCH_KIRKWOOD || ARCH_DOVE || ARCH_MVEBU
 	select WATCHDOG_CORE
 	help
 	  Say Y here if to include support for the watchdog timer

Signature · 1 line

1.8.1.5

Ezequiel Garcia

author

2014-01-21 13:26 UTC #23 lore raw

PATCH v3 15/15 ARM: mvebu: Enable watchdog support in defconfig

In reply to Ezequiel Garcia

Now that we have proper support for Armada 370/XP watchdog let's enable it in the defconfig.

Signed-off-by Ezequiel Garcia <[email protected]>

---
 arch/arm/configs/mvebu_defconfig | 2 ++
 1 file changed, 2 insertions(+)

arch/arm/configs/mvebu_defconfig +2 -0

index 594d706..84ec924 100644
@@ -60,6 +60,8 @@ CONFIG_GPIOLIB=y
 CONFIG_GPIO_SYSFS=y
 CONFIG_THERMAL=y
 CONFIG_ARMADA_THERMAL=y
+CONFIG_WATCHDOG=y
+CONFIG_ORION_WATCHDOG=y
 CONFIG_USB_SUPPORT=y
 CONFIG_USB=y
 CONFIG_USB_EHCI_HCD=y

Signature · 1 line

1.8.1.5