clocksource: Add brcm,bcm6345-timer device tree binding

Add device tree binding for the BCM6345 watchdog.

This uses the BCM6345 timer for its warning interrupt.

Signed-off-by Simon Arlott <[email protected]>

---
 .../bindings/watchdog/brcm,bcm6345-wdt.txt         | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/brcm,bcm6345-wdt.txt

Documentation/devicetree/bindings/watchdog/brcm,bcm6345-wdt.txt new file +35 -0

new file mode 100644
index 0000000..9d852d4
@@ -0,0 +1,35 @@
+BCM6345 Watchdog timer
+
+Required properties:
+
+- compatible: should be "brcm,bcm63<soc>-wdt", "brcm,bcm6345-wdt"
+- reg: Specifies base physical address and size of the registers.
+- clocks: Specify the clock used for timing
+
+Optional properties:
+
+- interrupt-parent: phandle to the interrupt controller
+- interrupts: Specify the interrupt used for the watchdog timout warning
+- timeout-sec: Contains the default watchdog timeout in seconds
+
+Example:
+
+watchdog {
+	compatible = "brcm,bcm63168-wdt", "brcm,bcm6345-wdt";
+	reg = <0x1000009c 0x0c>;
+	clocks = <&periph_clk>;
+
+	interrupt-parent = <&timer>;
+	interrupts = <3>;
+	timeout-sec = <30>;
+};
+
+watchdog {
+	compatible = "brcm,bcm6318-wdt", "brcm,bcm6345-wdt";
+	reg = <0x10000068 0x0c>;
+	clocks = <&periph_clk>;
+
+	interrupt-parent = <&timer>;
+	interrupts = <3>;
+	timeout-sec = <30>;
+};

Signature · 4 lines

2.1.4

-- 
Simon Arlott

Convert bcm63xx_wdt to use WATCHDOG_CORE and add a device tree binding.

Adds support for the time left value and provides a more effective interrupt handler based on the watchdog warning interrupt behaviour.

This removes the unnecessary software countdown timer and replaces the use of bcm63xx_timer with a normal interrupt when not using mach-bcm63xx.

Signed-off-by Simon Arlott <[email protected]>

---
  arch/mips/bcm63xx/prom.c                          |   1 +
  arch/mips/bcm63xx/setup.c                         |   1 +
  arch/mips/include/asm/mach-bcm63xx/bcm63xx_regs.h |  22 --
  drivers/watchdog/Kconfig                          |   4 +-
  drivers/watchdog/bcm63xx_wdt.c                    | 420 +++++++++++-----------
  include/linux/bcm63xx_wdt.h                       |  22 ++
  6 files changed, 244 insertions(+), 226 deletions(-)

create mode 100644 include/linux/bcm63xx_wdt.h

arch/mips/bcm63xx/prom.c +1 -0

index 7019e29..ba8b354 100644
@@ -17,6 +17,7 @@
  #include <bcm63xx_cpu.h>
  #include <bcm63xx_io.h>
  #include <bcm63xx_regs.h>
+#include <linux/bcm63xx_wdt.h>
 
  void __init prom_init(void)
  {

arch/mips/bcm63xx/setup.c +1 -0

index 240fb4f..6abf364 100644
@@ -21,6 +21,7 @@
  #include <bcm63xx_regs.h>
  #include <bcm63xx_io.h>
  #include <bcm63xx_gpio.h>
+#include <linux/bcm63xx_wdt.h>
 
  void bcm63xx_machine_halt(void)
  {

arch/mips/include/asm/mach-bcm63xx/bcm63xx_regs.h +0 -22

index 5035f09..16a745b 100644
@@ -441,28 +441,6 @@
 
 
  /*************************************************************************
- * _REG relative to RSET_WDT
- *************************************************************************/
-
-/* Watchdog default count register */
-#define WDT_DEFVAL_REG			0x0
-
-/* Watchdog control register */
-#define WDT_CTL_REG			0x4
-
-/* Watchdog control register constants */
-#define WDT_START_1			(0xff00)
-#define WDT_START_2			(0x00ff)
-#define WDT_STOP_1			(0xee00)
-#define WDT_STOP_2			(0x00ee)
-
-/* Watchdog reset length register */
-#define WDT_RSTLEN_REG			0x8
-
-/* Watchdog soft reset register (BCM6328 only) */
-#define WDT_SOFTRESET_REG		0xc
-
-/*************************************************************************
   * _REG relative to RSET_GPIO
   *************************************************************************/
 

drivers/watchdog/Kconfig +3 -1

index 7a8a6c6..0c50add 100644
@@ -1272,7 +1272,9 @@ config OCTEON_WDT
 
  config BCM63XX_WDT
  	tristate "Broadcom BCM63xx hardware watchdog"
-	depends on BCM63XX
+	depends on BCM63XX || BMIPS_GENERIC
+	select WATCHDOG_CORE
+	select BCM6345_L2_TIMER_IRQ if BMIPS_GENERIC
  	help
  	  Watchdog driver for the built in watchdog hardware in Broadcom
  	  BCM63xx SoC.

drivers/watchdog/bcm63xx_wdt.c +217 -203

index ab26fd9..fff92d0 100644
@@ -3,6 +3,7 @@
   *
   *  Copyright (C) 2007, Miguel Gaio <[email protected]>
   *  Copyright (C) 2008, Florian Fainelli <[email protected]>
+ *  Copyright 2015 Simon Arlott
   *
   *  This program is free software; you can redistribute it and/or
   *  modify it under the terms of the GNU General Public License

@@ -12,235 +13,165 @@
 
  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/bitops.h>
+#include <linux/bcm63xx_wdt.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
  #include <linux/errno.h>
-#include <linux/fs.h>
+#include <linux/interrupt.h>
  #include <linux/io.h>
  #include <linux/kernel.h>
-#include <linux/miscdevice.h>
  #include <linux/module.h>
  #include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/spinlock.h>
  #include <linux/types.h>
-#include <linux/uaccess.h>
  #include <linux/watchdog.h>
-#include <linux/timer.h>
-#include <linux/jiffies.h>
-#include <linux/interrupt.h>
-#include <linux/ptrace.h>
-#include <linux/resource.h>
-#include <linux/platform_device.h>
 
-#include <bcm63xx_cpu.h>
-#include <bcm63xx_io.h>
-#include <bcm63xx_regs.h>
-#include <bcm63xx_timer.h>
+#ifdef CONFIG_BCM63XX
+# include <bcm63xx_regs.h>
+# include <bcm63xx_timer.h>
+#endif
 
  #define PFX KBUILD_MODNAME
 
-#define WDT_HZ		50000000 /* Fclk */
-#define WDT_DEFAULT_TIME	30      /* seconds */
-#define WDT_MAX_TIME		256     /* seconds */
-
-static struct {
-	void __iomem *regs;
-	struct timer_list timer;
-	unsigned long inuse;
-	atomic_t ticks;
-} bcm63xx_wdt_device;
-
-static int expect_close;
-
-static int wdt_time = WDT_DEFAULT_TIME;
  static bool nowayout = WATCHDOG_NOWAYOUT;
  module_param(nowayout, bool, 0);
  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
  	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-/* HW functions */
-static void bcm63xx_wdt_hw_start(void)
-{
-	bcm_writel(0xfffffffe, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
-	bcm_writel(WDT_START_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-	bcm_writel(WDT_START_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-}
-
-static void bcm63xx_wdt_hw_stop(void)
-{
-	bcm_writel(WDT_STOP_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-	bcm_writel(WDT_STOP_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-}
-
-static void bcm63xx_wdt_isr(void *data)
-{
-	struct pt_regs *regs = get_irq_regs();
-
-	die(PFX " fire", regs);
-}
-
-static void bcm63xx_timer_tick(unsigned long unused)
-{
-	if (!atomic_dec_and_test(&bcm63xx_wdt_device.ticks)) {
-		bcm63xx_wdt_hw_start();
-		mod_timer(&bcm63xx_wdt_device.timer, jiffies + HZ);
-	} else
-		pr_crit("watchdog will restart system\n");
-}
-
-static void bcm63xx_wdt_pet(void)
-{
-	atomic_set(&bcm63xx_wdt_device.ticks, wdt_time);
-}
-
-static void bcm63xx_wdt_start(void)
-{
-	bcm63xx_wdt_pet();
-	bcm63xx_timer_tick(0);
-}
+struct bcm63xx_wdt_hw {
+	raw_spinlock_t lock;
+	void __iomem *base;
+	struct clk *clk;
+	u32 clock_hz;
+	int irq;
+	bool running;
+};
 
-static void bcm63xx_wdt_pause(void)
+static int bcm63xx_wdt_start(struct watchdog_device *wdd)
  {
-	del_timer_sync(&bcm63xx_wdt_device.timer);
-	bcm63xx_wdt_hw_stop();
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&hw->lock, flags);
+	__raw_writel(wdd->timeout * hw->clock_hz, hw->base + WDT_DEFVAL_REG);
+	__raw_writel(WDT_START_1, hw->base + WDT_CTL_REG);
+	__raw_writel(WDT_START_2, hw->base + WDT_CTL_REG);
+	hw->running = true;
+	raw_spin_unlock_irqrestore(&hw->lock, flags);
+	return 0;
  }
 
-static int bcm63xx_wdt_settimeout(int new_time)
+static int bcm63xx_wdt_stop(struct watchdog_device *wdd)
  {
-	if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
-		return -EINVAL;
-
-	wdt_time = new_time;
-
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&hw->lock, flags);
+	__raw_writel(WDT_STOP_1, hw->base + WDT_CTL_REG);
+	__raw_writel(WDT_STOP_2, hw->base + WDT_CTL_REG);
+	hw->running = false;
+	raw_spin_unlock_irqrestore(&hw->lock, flags);
  	return 0;
  }
 
-static int bcm63xx_wdt_open(struct inode *inode, struct file *file)
+static unsigned int bcm63xx_wdt_get_timeleft(struct watchdog_device *wdd)
  {
-	if (test_and_set_bit(0, &bcm63xx_wdt_device.inuse))
-		return -EBUSY;
-
-	bcm63xx_wdt_start();
-	return nonseekable_open(inode, file);
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&hw->lock, flags);
+	val = __raw_readl(hw->base + WDT_CTL_REG);
+	val /= hw->clock_hz;
+	raw_spin_unlock_irqrestore(&hw->lock, flags);
+	return val;
  }
 
-static int bcm63xx_wdt_release(struct inode *inode, struct file *file)
+static int bcm63xx_wdt_set_timeout(struct watchdog_device *wdd,
+	unsigned int timeout)
  {
-	if (expect_close == 42)
-		bcm63xx_wdt_pause();
-	else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		bcm63xx_wdt_start();
-	}
-	clear_bit(0, &bcm63xx_wdt_device.inuse);
-	expect_close = 0;
-	return 0;
+	wdd->timeout = timeout;
+	return bcm63xx_wdt_start(wdd);
  }
 
-static ssize_t bcm63xx_wdt_write(struct file *file, const char *data,
-				size_t len, loff_t *ppos)
+/* The watchdog interrupt occurs when half the timeout is remaining */
+#ifdef CONFIG_BCM63XX
+static void bcm63xx_wdt_interrupt(void *data)
+#else
+static irqreturn_t bcm63xx_wdt_interrupt(int irq, void *data)
+#endif
  {
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* In case it was set long ago */
-			expect_close = 0;
-
-			for (i = 0; i != len; i++) {
-				char c;
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_close = 42;
-			}
+	struct watchdog_device *wdd = data;
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&hw->lock, flags);
+	if (!hw->running) {
+		/* Oops */
+		__raw_writel(WDT_STOP_1, hw->base + WDT_CTL_REG);
+		__raw_writel(WDT_STOP_2, hw->base + WDT_CTL_REG);
+	} else {
+		u32 timeleft = __raw_readl(hw->base + WDT_CTL_REG);
+		u32 ms;
+
+		if (timeleft >= 2) {
+			/* The only way to stop this interrupt without masking
+			 * the whole timer interrupt or disrupting the intended
+			 * behaviour of the watchdog is to restart the watchdog
+			 * with the remaining time value so that the interrupt
+			 * occurs again at 1/4th, 1/8th, etc. of the timeout
+			 * until we reboot.
+			 *
+			 * This is done with a lock held in case userspace is
+			 * restarting the watchdog on another CPU.
+			 */
+			__raw_writel(timeleft, hw->base + WDT_DEFVAL_REG);
+			__raw_writel(WDT_START_1, hw->base + WDT_CTL_REG);
+			__raw_writel(WDT_START_2, hw->base + WDT_CTL_REG);
+		} else {
+			/* The watchdog cannot be started with a time of less
+			 * than 2 ticks (it won't fire).
+			 */
+			die(PFX ": watchdog timer expired\n", get_irq_regs());
  		}
-		bcm63xx_wdt_pet();
-	}
-	return len;
-}
-
-static struct watchdog_info bcm63xx_wdt_info = {
-	.identity       = PFX,
-	.options        = WDIOF_SETTIMEOUT |
-				WDIOF_KEEPALIVEPING |
-				WDIOF_MAGICCLOSE,
-};
-
-
-static long bcm63xx_wdt_ioctl(struct file *file, unsigned int cmd,
-				unsigned long arg)
-{
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	int new_value, retval = -EINVAL;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &bcm63xx_wdt_info,
-			sizeof(bcm63xx_wdt_info)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(0, p);
-
-	case WDIOC_SETOPTIONS:
-		if (get_user(new_value, p))
-			return -EFAULT;
-
-		if (new_value & WDIOS_DISABLECARD) {
-			bcm63xx_wdt_pause();
-			retval = 0;
-		}
-		if (new_value & WDIOS_ENABLECARD) {
-			bcm63xx_wdt_start();
-			retval = 0;
-		}
-
-		return retval;
-
-	case WDIOC_KEEPALIVE:
-		bcm63xx_wdt_pet();
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_value, p))
-			return -EFAULT;
-
-		if (bcm63xx_wdt_settimeout(new_value))
-			return -EINVAL;
-
-		bcm63xx_wdt_pet();
-
-	case WDIOC_GETTIMEOUT:
-		return put_user(wdt_time, p);
-
-	default:
-		return -ENOTTY;
 
+		ms = timeleft / (hw->clock_hz / 1000);
+		dev_alert(wdd->dev, "warning timer fired, reboot in %ums", ms);
  	}
+	raw_spin_unlock_irqrestore(&hw->lock, flags);
+#ifndef CONFIG_BCM63XX
+	return IRQ_HANDLED;
+#endif
  }
 
-static const struct file_operations bcm63xx_wdt_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= no_llseek,
-	.write		= bcm63xx_wdt_write,
-	.unlocked_ioctl	= bcm63xx_wdt_ioctl,
-	.open		= bcm63xx_wdt_open,
-	.release	= bcm63xx_wdt_release,
+static struct watchdog_ops bcm63xx_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = bcm63xx_wdt_start,
+	.stop = bcm63xx_wdt_stop,
+	.get_timeleft = bcm63xx_wdt_get_timeleft,
+	.set_timeout = bcm63xx_wdt_set_timeout,
  };
 
-static struct miscdevice bcm63xx_wdt_miscdev = {
-	.minor	= WATCHDOG_MINOR,
-	.name	= "watchdog",
-	.fops	= &bcm63xx_wdt_fops,
+static const struct watchdog_info bcm63xx_wdt_info = {
+	.options = WDIOC_GETTIMELEFT | WDIOF_SETTIMEOUT |
+			WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "BCM63xx Watchdog",
  };
 
-
  static int bcm63xx_wdt_probe(struct platform_device *pdev)
  {
-	int ret;
+	struct bcm63xx_wdt_hw *hw;
+	struct watchdog_device *wdd;
  	struct resource *r;
+	unsigned int timeleft;
+	int ret;
 
-	setup_timer(&bcm63xx_wdt_device.timer, bcm63xx_timer_tick, 0L);
+	hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL);
+	wdd = devm_kzalloc(&pdev->dev, sizeof(*wdd), GFP_KERNEL);
+	if (!hw || !wdd)
+		return -ENOMEM;
 
  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	if (!r) {

@@ -248,63 +179,145 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
  		return -ENODEV;
  	}
 
-	bcm63xx_wdt_device.regs = devm_ioremap_nocache(&pdev->dev, r->start,
-							resource_size(r));
-	if (!bcm63xx_wdt_device.regs) {
+	hw->base = devm_ioremap_nocache(&pdev->dev, r->start, resource_size(r));
+	if (!hw->base) {
  		dev_err(&pdev->dev, "failed to remap I/O resources\n");
  		return -ENXIO;
  	}
 
-	ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, NULL);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to register wdt timer isr\n");
+#ifdef CONFIG_BCM63XX
+	hw->clk = devm_clk_get(&pdev->dev, "periph");
+#else
+	hw->clk = devm_clk_get(&pdev->dev, NULL);
+#endif
+	if (IS_ERR(hw->clk)) {
+		dev_err(&pdev->dev, "unable to request clock\n");
+		return PTR_ERR(hw->clk);
+	}
+
+	hw->clock_hz = clk_get_rate(hw->clk);
+	if (!hw->clock_hz) {
+		dev_err(&pdev->dev, "unable to fetch clock rate\n");
+		return -EINVAL;
+	}
+
+	ret = clk_prepare_enable(hw->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to enable clock\n");
  		return ret;
  	}
 
-	if (bcm63xx_wdt_settimeout(wdt_time)) {
-		bcm63xx_wdt_settimeout(WDT_DEFAULT_TIME);
-		dev_info(&pdev->dev,
-			": wdt_time value must be 1 <= wdt_time <= 256, using %d\n",
-			wdt_time);
+	raw_spin_lock_init(&hw->lock);
+	hw->running = false;
+
+	wdd->parent = &pdev->dev;
+	wdd->ops = &bcm63xx_wdt_ops;
+	wdd->info = &bcm63xx_wdt_info;
+	wdd->min_timeout = 1;
+	wdd->max_timeout = 0xffffffff / hw->clock_hz;
+	wdd->timeout = min(30U, wdd->max_timeout);
+
+	watchdog_set_drvdata(wdd, hw);
+	platform_set_drvdata(pdev, wdd);
+
+	watchdog_init_timeout(wdd, 0, &pdev->dev);
+	watchdog_set_nowayout(wdd, nowayout);
+
+	timeleft = bcm63xx_wdt_get_timeleft(wdd);
+	if (timeleft > 0)
+		hw->running = true;
+
+#ifdef CONFIG_BCM63XX
+	ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_interrupt, wdd);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register with bcm63xx_timer\n");
+		goto disable_clk;
  	}
+	hw->irq = 0;
+#endif
 
-	ret = misc_register(&bcm63xx_wdt_miscdev);
+	ret = watchdog_register_device(wdd);
  	if (ret < 0) {
  		dev_err(&pdev->dev, "failed to register watchdog device\n");
+#ifdef CONFIG_BCM63XX
  		goto unregister_timer;
+#else
+		goto disable_clk;
+#endif
  	}
 
-	dev_info(&pdev->dev, " started, timer margin: %d sec\n",
-						WDT_DEFAULT_TIME);
+#ifndef CONFIG_BCM63XX
+	hw->irq = platform_get_irq(pdev, 0);
+	if (hw->irq) {
+		ret = devm_request_irq(&pdev->dev, hw->irq,
+			bcm63xx_wdt_interrupt, IRQF_TIMER,
+			dev_name(&pdev->dev), wdd);
+		if (ret)
+			hw->irq = 0;
+	}
+#endif
+
+	if (hw->irq) {
+		dev_info(&pdev->dev,
+			"%s at MMIO 0x%p (irq = %d, timeout = %us, max_timeout = %us)",
+			dev_name(wdd->dev), hw->base, hw->irq,
+			wdd->timeout, wdd->max_timeout);
+	} else {
+		dev_info(&pdev->dev,
+			"%s at MMIO 0x%p (timeout = %us, max_timeout = %us)",
+			dev_name(wdd->dev), hw->base,
+			wdd->timeout, wdd->max_timeout);
+	}
 
+	if (timeleft > 0)
+		dev_alert(wdd->dev, "running, reboot in %us\n", timeleft);
  	return 0;
 
+#ifdef CONFIG_BCM63XX
  unregister_timer:
  	bcm63xx_timer_unregister(TIMER_WDT_ID);
+#endif
+disable_clk:
+	clk_disable(hw->clk);
  	return ret;
  }
 
  static int bcm63xx_wdt_remove(struct platform_device *pdev)
  {
-	if (!nowayout)
-		bcm63xx_wdt_pause();
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
 
-	misc_deregister(&bcm63xx_wdt_miscdev);
+	if (hw->irq)
+		devm_free_irq(&pdev->dev, hw->irq, wdd);
+
+#ifdef CONFIG_BCM63XX
  	bcm63xx_timer_unregister(TIMER_WDT_ID);
+#endif
+	watchdog_unregister_device(wdd);
+	clk_disable(hw->clk);
  	return 0;
  }
 
  static void bcm63xx_wdt_shutdown(struct platform_device *pdev)
  {
-	bcm63xx_wdt_pause();
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
+
+	bcm63xx_wdt_stop(wdd);
  }
 
+static const struct of_device_id bcm63xx_wdt_dt_ids[] = {
+	{ .compatible = "brcm,bcm6345-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bcm63xx_wdt_dt_ids);
+
  static struct platform_driver bcm63xx_wdt_driver = {
  	.probe	= bcm63xx_wdt_probe,
  	.remove = bcm63xx_wdt_remove,
  	.shutdown = bcm63xx_wdt_shutdown,
  	.driver = {
  		.name = "bcm63xx-wdt",
+		.of_match_table = bcm63xx_wdt_dt_ids,
  	}
  };
 

@@ -312,6 +325,7 @@ module_platform_driver(bcm63xx_wdt_driver);
 
  MODULE_AUTHOR("Miguel Gaio <[email protected]>");
  MODULE_AUTHOR("Florian Fainelli <[email protected]>");
+MODULE_AUTHOR("Simon Arlott");
  MODULE_DESCRIPTION("Driver for the Broadcom BCM63xx SoC watchdog");
  MODULE_LICENSE("GPL");
  MODULE_ALIAS("platform:bcm63xx-wdt");

include/linux/bcm63xx_wdt.h new file +22 -0

new file mode 100644
index 0000000..ef4792e
@@ -0,0 +1,22 @@
+#ifndef LINUX_BCM63XX_WDT_H_
+#define LINUX_BCM63XX_WDT_H_
+
+/* Watchdog default count register */
+#define WDT_DEFVAL_REG			0x0
+
+/* Watchdog control register */
+#define WDT_CTL_REG			0x4
+
+/* Watchdog control register constants */
+#define WDT_START_1			(0xff00)
+#define WDT_START_2			(0x00ff)
+#define WDT_STOP_1			(0xee00)
+#define WDT_STOP_2			(0x00ee)
+
+/* Watchdog reset length register (in clock ticks) */
+#define WDT_RSTLEN_REG			0x8
+
+/* Watchdog soft reset register (BCM6328 only) */
+#define WDT_SOFTRESET_REG		0xc
+
+#endif

Quoted 8 lines · Simon Arlott

Convert bcm63xx_wdt to use WATCHDOG_CORE and add a device tree binding.

Adds support for the time left value and provides a more effective interrupt handler based on the watchdog warning interrupt behaviour.

This removes the unnecessary software countdown timer and replaces the use of bcm63xx_timer with a normal interrupt when not using mach-bcm63xx.

Hi Simon,

this is really doing a bit too much in a single patch. Conversion to the watchdog infrastructure should probably be the first step, followed by further optimizations and improvements.

In general, it would be great if we can avoid #ifdef in the code. Maybe there is some other means to determine if one code path needs to be taken or another. The driver may be part of a multi-platform image, and #ifdefs in the code make that all but impossible. Besides, it makes the code really hard to read and understand.

We have some infrastructure changes in the works which will move the need for soft-timers from individual drivers into the watchdog core. Would this possibly be helpful here ? The timer-driven watchdog ping seems to accomplish pretty much the same.

Quoted 10 lines · Guenter Roeck

Quoted 8 lines · Simon Arlott

Convert bcm63xx_wdt to use WATCHDOG_CORE and add a device tree binding.

Adds support for the time left value and provides a more effective interrupt handler based on the watchdog warning interrupt behaviour.

This removes the unnecessary software countdown timer and replaces the use of bcm63xx_timer with a normal interrupt when not using mach-bcm63xx.

Quoted 5 lines

Hi Simon,

this is really doing a bit too much in a single patch. Conversion to the watchdog infrastructure should probably be the first step, followed by further optimizations and improvements.

I'll split it into two patches, but that won't remove the need for #ifdefs.

Quoted 6 lines

In general, it would be great if we can avoid #ifdef in the code. Maybe there is some other means to determine if one code path needs to be taken or another. The driver may be part of a multi-platform image, and #ifdefs in the code make that all but impossible. Besides, it makes the code really hard to read and understand.

It's impossible to avoid the #ifdefs because the driver needs to support mach-bmips while still supporting mach-bcm63xx. I don't think they make it too difficult to understand. Until there are device tree supporting drivers for everything mach-bcm63xx needs, it can't be removed.

Quoted 4 lines

We have some infrastructure changes in the works which will move the need for soft-timers from individual drivers into the watchdog core. Would this possibly be helpful here ? The timer-driven watchdog ping seems to accomplish pretty much the same.

There is no need for a software timer. This is not a timer-driven watchdog ping, there is an unmaskable timer interrupt when the watchdog timer has less than 50% remaining.

Quoted 5 lines · Simon Arlott

Quoted 3 lines · Guenter Roeck

this is really doing a bit too much in a single patch. Conversion to the watchdog infrastructure should probably be the first step, followed by further optimizations and improvements.

Quoted 5 lines

Quoted 4 lines

We have some infrastructure changes in the works which will move the need for soft-timers from individual drivers into the watchdog core. Would this possibly be helpful here ? The timer-driven watchdog ping seems to accomplish pretty much the same.

Quoted 4 lines

There is no need for a software timer. This is not a timer-driven watchdog ping, there is an unmaskable timer interrupt when the watchdog timer has less than 50% remaining.

Ok. Maybe I got confused by the interrupt-triggered watchdog ping. I'll have to look into that much more closely; it is quite unusual and complex. The explanation is also not easy to understand. What does "The only way to stop this interrupt" mean ? Repeatedly

triggering the interrupt in 1/2, 1/4, 1/8 of the remaining time is really odd.

On side note, the subject tag should be "watchdog:", not "MIPS:".

There is a level triggered interrupt for the watchdog timer as part of the bcm63xx_timer device. The interrupt occurs when the hardware watchdog timer reaches 50% of the remaining time.

It is not possible to mask the interrupt within the bcm63xx_timer device. To get around this limitation, handle the interrupt by restarting the watchdog with the current remaining time (which will be half the previous timeout) so that the interrupt occurs again at 1/4th, 1/8th, etc. of the original timeout value until the watchdog forces a reboot.

The software timer was restarting the hardware watchdog with a 85 second timeout until the software timer expired, and then causing a panic() about 42.5 seconds later when the hardware interrupt occurred. The hardware watchdog would not reboot until a further 42.5 seconds had passed.

Remove the software timer and rely on the hardware timer directly, reducing the maximum timeout from 256 seconds to 85 seconds (2^32 / WDT_HZ).

Signed-off-by Simon Arlott <[email protected]>

---
 drivers/watchdog/bcm63xx_wdt.c | 124 ++++++++++++++++++++++++-----------------
 1 file changed, 72 insertions(+), 52 deletions(-)

drivers/watchdog/bcm63xx_wdt.c +72 -52

index ab26fd9..f88fc97 100644
@@ -3,6 +3,7 @@
  *
  *  Copyright (C) 2007, Miguel Gaio <[email protected]>
  *  Copyright (C) 2008, Florian Fainelli <[email protected]>
+ *  Copyright 2015 Simon Arlott
  *
  *  This program is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU General Public License

@@ -20,11 +21,10 @@
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
 #include <linux/watchdog.h>
-#include <linux/timer.h>
-#include <linux/jiffies.h>
 #include <linux/interrupt.h>
 #include <linux/ptrace.h>
 #include <linux/resource.h>

@@ -37,16 +37,17 @@
 
 #define PFX KBUILD_MODNAME
 
-#define WDT_HZ		50000000 /* Fclk */
-#define WDT_DEFAULT_TIME	30      /* seconds */
-#define WDT_MAX_TIME		256     /* seconds */
+#define WDT_HZ			50000000		/* Fclk */
+#define WDT_DEFAULT_TIME	30			/* seconds */
+#define WDT_MAX_TIME		(0xffffffff / WDT_HZ)	/* seconds */
 
-static struct {
+struct bcm63xx_wdt_hw {
+	raw_spinlock_t lock;
 	void __iomem *regs;
-	struct timer_list timer;
 	unsigned long inuse;
-	atomic_t ticks;
-} bcm63xx_wdt_device;
+	bool running;
+};
+static struct bcm63xx_wdt_hw bcm63xx_wdt_device;
 
 static int expect_close;
 

@@ -59,48 +60,67 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
 /* HW functions */
 static void bcm63xx_wdt_hw_start(void)
 {
-	bcm_writel(0xfffffffe, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
+	bcm_writel(wdt_time * WDT_HZ, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
 	bcm_writel(WDT_START_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
 	bcm_writel(WDT_START_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
+	bcm63xx_wdt_device.running = true;
+	raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
 }
 
 static void bcm63xx_wdt_hw_stop(void)
 {
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
 	bcm_writel(WDT_STOP_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
 	bcm_writel(WDT_STOP_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
+	bcm63xx_wdt_device.running = false;
+	raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
 }
 
+/* The watchdog interrupt occurs when half the timeout is remaining */
 static void bcm63xx_wdt_isr(void *data)
 {
-	struct pt_regs *regs = get_irq_regs();
-
-	die(PFX " fire", regs);
-}
-
-static void bcm63xx_timer_tick(unsigned long unused)
-{
-	if (!atomic_dec_and_test(&bcm63xx_wdt_device.ticks)) {
-		bcm63xx_wdt_hw_start();
-		mod_timer(&bcm63xx_wdt_device.timer, jiffies + HZ);
-	} else
-		pr_crit("watchdog will restart system\n");
-}
-
-static void bcm63xx_wdt_pet(void)
-{
-	atomic_set(&bcm63xx_wdt_device.ticks, wdt_time);
-}
-
-static void bcm63xx_wdt_start(void)
-{
-	bcm63xx_wdt_pet();
-	bcm63xx_timer_tick(0);
-}
+	struct bcm63xx_wdt_hw *hw = &bcm63xx_wdt_device;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&hw->lock, flags);
+	if (!hw->running) {
+		/* Stop the watchdog as it shouldn't be running */
+		bcm_writel(WDT_STOP_1, hw->regs + WDT_CTL_REG);
+		bcm_writel(WDT_STOP_2, hw->regs + WDT_CTL_REG);
+	} else {
+		u32 timeleft = bcm_readl(hw->regs + WDT_CTL_REG);
+		u32 ms;
+
+		if (timeleft >= 2) {
+			/* The only way to clear this level triggered interrupt
+			 * without disrupting the normal running of the watchdog
+			 * is to restart the watchdog with the current remaining
+			 * time value (which will be half the previous timeout)
+			 * so the interrupt occurs again at 1/4th, 1/8th, etc.
+			 * of the original timeout value until we reboot.
+			 *
+			 * This is done with a lock held in case userspace is
+			 * trying to restart the watchdog on another CPU.
+			 */
+			bcm_writel(timeleft, hw->regs + WDT_DEFVAL_REG);
+			bcm_writel(WDT_START_1, hw->regs + WDT_CTL_REG);
+			bcm_writel(WDT_START_2, hw->regs + WDT_CTL_REG);
+		} else {
+			/* The watchdog cannot be started with a time of less
+			 * than 2 ticks (it won't fire).
+			 */
+			die(PFX ": watchdog timer expired\n", get_irq_regs());
+		}
 
-static void bcm63xx_wdt_pause(void)
-{
-	del_timer_sync(&bcm63xx_wdt_device.timer);
-	bcm63xx_wdt_hw_stop();
+		ms = timeleft / (WDT_HZ / 1000);
+		pr_alert("warning timer fired, reboot in %ums\n", ms);
+	}
+	raw_spin_unlock_irqrestore(&hw->lock, flags);
 }
 
 static int bcm63xx_wdt_settimeout(int new_time)

@@ -118,17 +138,17 @@ static int bcm63xx_wdt_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(0, &bcm63xx_wdt_device.inuse))
 		return -EBUSY;
 
-	bcm63xx_wdt_start();
+	bcm63xx_wdt_hw_start();
 	return nonseekable_open(inode, file);
 }
 
 static int bcm63xx_wdt_release(struct inode *inode, struct file *file)
 {
 	if (expect_close == 42)
-		bcm63xx_wdt_pause();
+		bcm63xx_wdt_hw_stop();
 	else {
 		pr_crit("Unexpected close, not stopping watchdog!\n");
-		bcm63xx_wdt_start();
+		bcm63xx_wdt_hw_start();
 	}
 	clear_bit(0, &bcm63xx_wdt_device.inuse);
 	expect_close = 0;

@@ -153,7 +173,7 @@ static ssize_t bcm63xx_wdt_write(struct file *file, const char *data,
 					expect_close = 42;
 			}
 		}
-		bcm63xx_wdt_pet();
+		bcm63xx_wdt_hw_start();
 	}
 	return len;
 }

@@ -187,18 +207,18 @@ static long bcm63xx_wdt_ioctl(struct file *file, unsigned int cmd,
 			return -EFAULT;
 
 		if (new_value & WDIOS_DISABLECARD) {
-			bcm63xx_wdt_pause();
+			bcm63xx_wdt_hw_stop();
 			retval = 0;
 		}
 		if (new_value & WDIOS_ENABLECARD) {
-			bcm63xx_wdt_start();
+			bcm63xx_wdt_hw_start();
 			retval = 0;
 		}
 
 		return retval;
 
 	case WDIOC_KEEPALIVE:
-		bcm63xx_wdt_pet();
+		bcm63xx_wdt_hw_start();
 		return 0;
 
 	case WDIOC_SETTIMEOUT:

@@ -208,7 +228,7 @@ static long bcm63xx_wdt_ioctl(struct file *file, unsigned int cmd,
 		if (bcm63xx_wdt_settimeout(new_value))
 			return -EINVAL;
 
-		bcm63xx_wdt_pet();
+		bcm63xx_wdt_hw_start();
 
 	case WDIOC_GETTIMEOUT:
 		return put_user(wdt_time, p);

@@ -240,8 +260,6 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
 	int ret;
 	struct resource *r;
 
-	setup_timer(&bcm63xx_wdt_device.timer, bcm63xx_timer_tick, 0L);
-
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!r) {
 		dev_err(&pdev->dev, "failed to get resources\n");

@@ -255,6 +273,8 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
+	raw_spin_lock_init(&bcm63xx_wdt_device.lock);
+
 	ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, NULL);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to register wdt timer isr\n");

@@ -264,8 +284,8 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
 	if (bcm63xx_wdt_settimeout(wdt_time)) {
 		bcm63xx_wdt_settimeout(WDT_DEFAULT_TIME);
 		dev_info(&pdev->dev,
-			": wdt_time value must be 1 <= wdt_time <= 256, using %d\n",
-			wdt_time);
+			": wdt_time value must be 1 <= wdt_time <= %d, using %d\n",
+			WDT_MAX_TIME, wdt_time);
 	}
 
 	ret = misc_register(&bcm63xx_wdt_miscdev);

@@ -287,7 +307,7 @@ unregister_timer:
 static int bcm63xx_wdt_remove(struct platform_device *pdev)
 {
 	if (!nowayout)
-		bcm63xx_wdt_pause();
+		bcm63xx_wdt_hw_stop();
 
 	misc_deregister(&bcm63xx_wdt_miscdev);
 	bcm63xx_timer_unregister(TIMER_WDT_ID);

@@ -296,7 +316,7 @@ static int bcm63xx_wdt_remove(struct platform_device *pdev)
 
 static void bcm63xx_wdt_shutdown(struct platform_device *pdev)
 {
-	bcm63xx_wdt_pause();
+	bcm63xx_wdt_hw_stop();
 }
 
 static struct platform_driver bcm63xx_wdt_driver = {

Signature · 4 lines

2.1.4

-- 
Simon Arlott

Quoted 16 lines · Simon Arlott

There is a level triggered interrupt for the watchdog timer as part of the bcm63xx_timer device. The interrupt occurs when the hardware watchdog timer reaches 50% of the remaining time.

It is not possible to mask the interrupt within the bcm63xx_timer device. To get around this limitation, handle the interrupt by restarting the watchdog with the current remaining time (which will be half the previous timeout) so that the interrupt occurs again at 1/4th, 1/8th, etc. of the original timeout value until the watchdog forces a reboot.

The software timer was restarting the hardware watchdog with a 85 second timeout until the software timer expired, and then causing a panic() about 42.5 seconds later when the hardware interrupt occurred. The hardware watchdog would not reboot until a further 42.5 seconds had passed.

Quoted 4 lines

Remove the software timer and rely on the hardware timer directly, reducing the maximum timeout from 256 seconds to 85 seconds (2^32 / WDT_HZ).

Florian,

can you have a look into this patch and confirm that there is no better way to clear the interrupt status ?

Quoted 22 lines · Guenter Roeck

Quoted 20 lines · Simon Arlott

There is a level triggered interrupt for the watchdog timer as part of the bcm63xx_timer device. The interrupt occurs when the hardware watchdog timer reaches 50% of the remaining time.

It is not possible to mask the interrupt within the bcm63xx_timer device. To get around this limitation, handle the interrupt by restarting the watchdog with the current remaining time (which will be half the previous timeout) so that the interrupt occurs again at 1/4th, 1/8th, etc. of the original timeout value until the watchdog forces a reboot.

The software timer was restarting the hardware watchdog with a 85 second timeout until the software timer expired, and then causing a panic() about 42.5 seconds later when the hardware interrupt occurred. The hardware watchdog would not reboot until a further 42.5 seconds had passed.

Remove the software timer and rely on the hardware timer directly, reducing the maximum timeout from 256 seconds to 85 seconds (2^32 / WDT_HZ).

Quoted 4 lines

Florian,

can you have a look into this patch and confirm that there is no better way to clear the interrupt status ?

While the watchdog interrupt can't be masked, it should be able to be cleared by writing 1 to the appropriate bit in the timer block's interrupt status register. At least the broadcom sources do so.

Convert bcm63xx_wdt to use WATCHDOG_CORE.

The default and maximum time constants that are only used once have been moved to the initialisation of the struct watchdog_device.

Signed-off-by Simon Arlott <[email protected]>

---
  drivers/watchdog/Kconfig       |   1 +
  drivers/watchdog/bcm63xx_wdt.c | 249 ++++++++++++-----------------------------
  2 files changed, 74 insertions(+), 176 deletions(-)

drivers/watchdog/Kconfig +1 -0

index 7a8a6c6..6815b74 100644
@@ -1273,6 +1273,7 @@ config OCTEON_WDT
  config BCM63XX_WDT
  	tristate "Broadcom BCM63xx hardware watchdog"
  	depends on BCM63XX
+	select WATCHDOG_CORE
  	help
  	  Watchdog driver for the built in watchdog hardware in Broadcom
  	  BCM63xx SoC.

drivers/watchdog/bcm63xx_wdt.c +0 -5

index f88fc97..1d2a501 100644
@@ -13,20 +13,15 @@
 
  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/bitops.h>
  #include <linux/errno.h>
-#include <linux/fs.h>
  #include <linux/io.h>
  #include <linux/kernel.h>
-#include <linux/miscdevice.h>
  #include <linux/module.h>
  #include <linux/moduleparam.h>
  #include <linux/spinlock.h>
  #include <linux/types.h>
-#include <linux/uaccess.h>
  #include <linux/watchdog.h>
  #include <linux/interrupt.h>
-#include <linux/ptrace.h>
  #include <linux/resource.h>
  #include <linux/platform_device.h>
 

@@ -38,53 +33,57 @@
  #define PFX KBUILD_MODNAME

drivers/watchdog/bcm63xx_wdt.c +14 -18

  };
-static struct bcm63xx_wdt_hw bcm63xx_wdt_device;
 
-static int expect_close;
-
-static int wdt_time = WDT_DEFAULT_TIME;
  static bool nowayout = WATCHDOG_NOWAYOUT;
  module_param(nowayout, bool, 0);
  MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
  	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
 
-/* HW functions */
-static void bcm63xx_wdt_hw_start(void)
+static int bcm63xx_wdt_start(struct watchdog_device *wdd)
  {
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
  	unsigned long flags;
 
-	raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
-	bcm_writel(wdt_time * WDT_HZ, bcm63xx_wdt_device.regs + WDT_DEFVAL_REG);
-	bcm_writel(WDT_START_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-	bcm_writel(WDT_START_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-	bcm63xx_wdt_device.running = true;
-	raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
+	raw_spin_lock_irqsave(&hw->lock, flags);
+	bcm_writel(wdd->timeout * WDT_HZ, hw->regs + WDT_DEFVAL_REG);
+	bcm_writel(WDT_START_1, hw->regs + WDT_CTL_REG);
+	bcm_writel(WDT_START_2, hw->regs + WDT_CTL_REG);
+	hw->running = true;
+	raw_spin_unlock_irqrestore(&hw->lock, flags);
+	return 0;
  }
 
-static void bcm63xx_wdt_hw_stop(void)
+static int bcm63xx_wdt_stop(struct watchdog_device *wdd)
  {
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
  	unsigned long flags;
 
-	raw_spin_lock_irqsave(&bcm63xx_wdt_device.lock, flags);
-	bcm_writel(WDT_STOP_1, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-	bcm_writel(WDT_STOP_2, bcm63xx_wdt_device.regs + WDT_CTL_REG);
-	bcm63xx_wdt_device.running = false;
-	raw_spin_unlock_irqrestore(&bcm63xx_wdt_device.lock, flags);
+	raw_spin_lock_irqsave(&hw->lock, flags);
+	bcm_writel(WDT_STOP_1, hw->regs + WDT_CTL_REG);
+	bcm_writel(WDT_STOP_2, hw->regs + WDT_CTL_REG);

drivers/watchdog/bcm63xx_wdt.c +7 -125

  }
 
  /* The watchdog interrupt occurs when half the timeout is remaining */
  static void bcm63xx_wdt_isr(void *data)
  {
-	struct bcm63xx_wdt_hw *hw = &bcm63xx_wdt_device;
+	struct watchdog_device *wdd = data;
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
  	unsigned long flags;
 
  	raw_spin_lock_irqsave(&hw->lock, flags);

@@ -118,147 +117,36 @@ static void bcm63xx_wdt_isr(void *data)
  		}
 
  		ms = timeleft / (WDT_HZ / 1000);
-		pr_alert("warning timer fired, reboot in %ums\n", ms);
+		dev_alert(wdd->dev,
+			"warning timer fired, reboot in %ums\n", ms);
  	}
  	raw_spin_unlock_irqrestore(&hw->lock, flags);
  }
 
-static int bcm63xx_wdt_settimeout(int new_time)
-{
-	if ((new_time <= 0) || (new_time > WDT_MAX_TIME))
-		return -EINVAL;
-
-	wdt_time = new_time;
-
-	return 0;
-}
-
-static int bcm63xx_wdt_open(struct inode *inode, struct file *file)
-{
-	if (test_and_set_bit(0, &bcm63xx_wdt_device.inuse))
-		return -EBUSY;
-
-	bcm63xx_wdt_hw_start();
-	return nonseekable_open(inode, file);
-}
-
-static int bcm63xx_wdt_release(struct inode *inode, struct file *file)
-{
-	if (expect_close == 42)
-		bcm63xx_wdt_hw_stop();
-	else {
-		pr_crit("Unexpected close, not stopping watchdog!\n");
-		bcm63xx_wdt_hw_start();
-	}
-	clear_bit(0, &bcm63xx_wdt_device.inuse);
-	expect_close = 0;
-	return 0;
-}
-
-static ssize_t bcm63xx_wdt_write(struct file *file, const char *data,
-				size_t len, loff_t *ppos)
-{
-	if (len) {
-		if (!nowayout) {
-			size_t i;
-
-			/* In case it was set long ago */
-			expect_close = 0;
-
-			for (i = 0; i != len; i++) {
-				char c;
-				if (get_user(c, data + i))
-					return -EFAULT;
-				if (c == 'V')
-					expect_close = 42;
-			}
-		}
-		bcm63xx_wdt_hw_start();
-	}
-	return len;
-}
-
-static struct watchdog_info bcm63xx_wdt_info = {
-	.identity       = PFX,
-	.options        = WDIOF_SETTIMEOUT |
-				WDIOF_KEEPALIVEPING |
-				WDIOF_MAGICCLOSE,
-};
-
-
-static long bcm63xx_wdt_ioctl(struct file *file, unsigned int cmd,
-				unsigned long arg)
-{
-	void __user *argp = (void __user *)arg;
-	int __user *p = argp;
-	int new_value, retval = -EINVAL;
-
-	switch (cmd) {
-	case WDIOC_GETSUPPORT:
-		return copy_to_user(argp, &bcm63xx_wdt_info,
-			sizeof(bcm63xx_wdt_info)) ? -EFAULT : 0;
-
-	case WDIOC_GETSTATUS:
-	case WDIOC_GETBOOTSTATUS:
-		return put_user(0, p);
-
-	case WDIOC_SETOPTIONS:
-		if (get_user(new_value, p))
-			return -EFAULT;
-
-		if (new_value & WDIOS_DISABLECARD) {
-			bcm63xx_wdt_hw_stop();
-			retval = 0;
-		}
-		if (new_value & WDIOS_ENABLECARD) {
-			bcm63xx_wdt_hw_start();
-			retval = 0;
-		}
-
-		return retval;
-
-	case WDIOC_KEEPALIVE:
-		bcm63xx_wdt_hw_start();
-		return 0;
-
-	case WDIOC_SETTIMEOUT:
-		if (get_user(new_value, p))
-			return -EFAULT;
-
-		if (bcm63xx_wdt_settimeout(new_value))
-			return -EINVAL;
-
-		bcm63xx_wdt_hw_start();
-
-	case WDIOC_GETTIMEOUT:
-		return put_user(wdt_time, p);
-
-	default:
-		return -ENOTTY;
-
-	}
-}
-
-static const struct file_operations bcm63xx_wdt_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= no_llseek,
-	.write		= bcm63xx_wdt_write,
-	.unlocked_ioctl	= bcm63xx_wdt_ioctl,
-	.open		= bcm63xx_wdt_open,
-	.release	= bcm63xx_wdt_release,
+static struct watchdog_ops bcm63xx_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = bcm63xx_wdt_start,

drivers/watchdog/bcm63xx_wdt.c +2 -1

+			WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+	.identity = "BCM63xx Watchdog",
  };
 
-

drivers/watchdog/bcm63xx_wdt.c +25 -15

+	if (!hw || !wdd)
+		return -ENOMEM;
 
  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	if (!r) {

@@ -266,36 +154,44 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
  		return -ENODEV;
  	}
 
-	bcm63xx_wdt_device.regs = devm_ioremap_nocache(&pdev->dev, r->start,
-							resource_size(r));
-	if (!bcm63xx_wdt_device.regs) {
+	hw->regs = devm_ioremap_nocache(&pdev->dev, r->start, resource_size(r));
+	if (!hw->regs) {
  		dev_err(&pdev->dev, "failed to remap I/O resources\n");
  		return -ENXIO;
  	}
 
-	raw_spin_lock_init(&bcm63xx_wdt_device.lock);
+	raw_spin_lock_init(&hw->lock);
+	hw->running = false;
+
+	wdd->parent = &pdev->dev;
+	wdd->ops = &bcm63xx_wdt_ops;
+	wdd->info = &bcm63xx_wdt_info;
+	wdd->min_timeout = 1;
+	wdd->max_timeout = 0xffffffff / WDT_HZ;
+	wdd->timeout = min(30U, wdd->max_timeout);
+
+	watchdog_set_drvdata(wdd, hw);
+	platform_set_drvdata(pdev, wdd);
+
+	watchdog_init_timeout(wdd, 0, &pdev->dev);
+	watchdog_set_nowayout(wdd, nowayout);
 
-	ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, NULL);
+	ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, wdd);
  	if (ret < 0) {
  		dev_err(&pdev->dev, "failed to register wdt timer isr\n");
  		return ret;
  	}
 
-	if (bcm63xx_wdt_settimeout(wdt_time)) {
-		bcm63xx_wdt_settimeout(WDT_DEFAULT_TIME);
-		dev_info(&pdev->dev,
-			": wdt_time value must be 1 <= wdt_time <= %d, using %d\n",
-			WDT_MAX_TIME, wdt_time);
-	}
-
-	ret = misc_register(&bcm63xx_wdt_miscdev);
+	ret = watchdog_register_device(wdd);
  	if (ret < 0) {
  		dev_err(&pdev->dev, "failed to register watchdog device\n");
  		goto unregister_timer;
  	}
 
-	dev_info(&pdev->dev, " started, timer margin: %d sec\n",
-						WDT_DEFAULT_TIME);
+	dev_info(&pdev->dev,
+		"%s at MMIO 0x%p (timeout = %us, max_timeout = %us)",
+		dev_name(wdd->dev), hw->regs,
+		wdd->timeout, wdd->max_timeout);
 
  	return 0;
 

The "running" flag should no longer be needed. watchdog_active() should provide that information.

Quoted 8 lines

drivers/watchdog/bcm63xx_wdt.c +1 -2

@@ -306,17 +202,18 @@ unregister_timer:
 
  static int bcm63xx_wdt_remove(struct platform_device *pdev)
  {
-	if (!nowayout)
-		bcm63xx_wdt_hw_stop();
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
 

drivers/watchdog/bcm63xx_wdt.c unregister_timer: · line 208

Quoted 3 lines

drivers/watchdog/bcm63xx_wdt.c +1 -1

-	misc_deregister(&bcm63xx_wdt_miscdev);
  	bcm63xx_timer_unregister(TIMER_WDT_ID);
+	watchdog_unregister_device(wdd);

Shouldn't that come first, before unregistering the timer ?

Quoted 2 lines · Guenter Roeck

The "running" flag should no longer be needed. watchdog_active() should provide that information.

I'm going to need to keep that because I need to know if it's running in the interrupt handler, and wdd->lock is a mutex.

Quoted 8 lines

Quoted 8 lines

drivers/watchdog/bcm63xx_wdt.c +1 -2

@@ -306,17 +202,18 @@ unregister_timer:
 
   static int bcm63xx_wdt_remove(struct platform_device *pdev)
   {
-	if (!nowayout)
-		bcm63xx_wdt_hw_stop();
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
 

Quoted 5 lines

drivers/watchdog/bcm63xx_wdt.c unregister_timer: · line 208

Quoted 3 lines

drivers/watchdog/bcm63xx_wdt.c +1 -1

-	misc_deregister(&bcm63xx_wdt_miscdev);
   	bcm63xx_timer_unregister(TIMER_WDT_ID);
+	watchdog_unregister_device(wdd);

Shouldn't that come first, before unregistering the timer ?

No, because wdd->dev is used in the interrupt handler. I will have to move registration of the interrupt to after creating the watchdog because it could currently be used before wdd->dev is set.

Quoted 21 lines · Simon Arlott

Quoted 2 lines · Guenter Roeck

The "running" flag should no longer be needed. watchdog_active() should provide that information.

I'm going to need to keep that because I need to know if it's running in the interrupt handler, and wdd->lock is a mutex.

Quoted 13 lines

drivers/watchdog/bcm63xx_wdt.c unregister_timer: · line 208

Quoted 11 lines

drivers/watchdog/bcm63xx_wdt.c +2 -3

@@ -306,17 +202,18 @@ unregister_timer:
 
   static int bcm63xx_wdt_remove(struct platform_device *pdev)
   {
-	if (!nowayout)
-		bcm63xx_wdt_hw_stop();
+	struct watchdog_device *wdd = platform_get_drvdata(pdev);
 
-	misc_deregister(&bcm63xx_wdt_miscdev);
   	bcm63xx_timer_unregister(TIMER_WDT_ID);
+	watchdog_unregister_device(wdd);

Shouldn't that come first, before unregistering the timer ?

Quoted 4 lines

No, because wdd->dev is used in the interrupt handler. I will have to move registration of the interrupt to after creating the watchdog because it could currently be used before wdd->dev is set.

Does unregistering the timer disable the interrupt ?

If I see correctly, there is no ping function. In that case, the watchdog core will call the start function after updating the timeout, so there is no need to do it here.

Review

Quoted 2 lines

(quoted diff) +2 -0

+static const struct watchdog_info bcm63xx_wdt_info = {
+	.options = WDIOC_GETTIMELEFT | WDIOF_SETTIMEOUT |

Where is the gettimeleft function ? I think you are adding it with a later patch, but then you should set the flag there, not here.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL);
+	wdd = devm_kzalloc(&pdev->dev, sizeof(*wdd), GFP_KERNEL);

It would be better to allocate wdd as part of struct bcm63xx_wdt_hw.

Review

Quoted 3 lines

(quoted diff) +1 -1

-	misc_deregister(&bcm63xx_wdt_miscdev);
  	bcm63xx_timer_unregister(TIMER_WDT_ID);
+	watchdog_unregister_device(wdd);

Shouldn't that come first, before unregistering the timer ?

Instead of using a fixed clock HZ in the driver, obtain it from the "periph" clk that the watchdog timer uses.

Signed-off-by Simon Arlott <[email protected]>

---
 drivers/watchdog/bcm63xx_wdt.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

drivers/watchdog/bcm63xx_wdt.c +1 -0

index 1d2a501..eb5e551 100644
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/kernel.h>

Hi,

Quoted 24 lines · Simon Arlott

Instead of using a fixed clock HZ in the driver, obtain it from the "periph" clk that the watchdog timer uses.

Signed-off-by Simon Arlott <[email protected]>

---
 drivers/watchdog/bcm63xx_wdt.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

drivers/watchdog/bcm63xx_wdt.c +1 -0

index 1d2a501..eb5e551 100644
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/kernel.h>

@@ -32,11 +33,13 @@
 
 #define PFX KBUILD_MODNAME
 

drivers/watchdog/bcm63xx_wdt.c line 36

Quoted 2 lines

drivers/watchdog/bcm63xx_wdt.c +1 -1

-#define WDT_HZ                 50000000                /* Fclk */
+#define WDT_CLK_NAME           "periph"

@Florian:
Is this correct? The comment for the watchdog in 6358_map_part.h and earlier claims that the clock is 40 MHz there, but the code uses 50MHz
- is this a bug in the comments or is it a bug taken over from the original broadcom code? I'm sure that the periph clock being 50 MHz even on the older chips is correct, else we'd have noticed that in serial output (where it's also used).

Quoted 11 lines · Jonas Gorski

drivers/watchdog/bcm63xx_wdt.c line 36

Quoted 2 lines · Simon Arlott

drivers/watchdog/bcm63xx_wdt.c +1 -1

-#define WDT_HZ                 50000000                /* Fclk */
+#define WDT_CLK_NAME           "periph"

@Florian:
Is this correct? The comment for the watchdog in 6358_map_part.h and earlier claims that the clock is 40 MHz there, but the code uses 50MHz
- is this a bug in the comments or is it a bug taken over from the original broadcom code? I'm sure that the periph clock being 50 MHz even on the older chips is correct, else we'd have noticed that in serial output (where it's also used).

There are references to a Fbus2 clock in documentation, but I could not find any actual documentation for its actual clock frequency, I would be surprised if this chip would have diverged from the previous and future ones and used a 40Mhz clock. 6345 started with a peripheral clock running at 50Mhz, and that is true for all chips since then AFAICT.

I agree we would have noticed this with the UART or SPI controllers if that was not true, so probably a code glitch here...

Instead of using a fixed clock HZ in the driver, obtain it from the "periph" clk that the watchdog timer uses.

Signed-off-by Simon Arlott <[email protected]>

Quoted 4 lines · Simon Arlott

Instead of using a fixed clock HZ in the driver, obtain it from the "periph" clk that the watchdog timer uses.

Signed-off-by Simon Arlott <[email protected]>

Reviewed-by Florian Fainelli <[email protected]>

Return the remaining time from the hardware control register.

Warn when the device is registered if the hardware watchdog is currently running and report the remaining time left.

Signed-off-by Simon Arlott <[email protected]>

Return the remaining time from the hardware control register.

Warn when the device is registered if the hardware watchdog is currently running and report the remaining time left.

Signed-off-by Simon Arlott <[email protected]>

---
Changed "if (timeleft > 0)" to "if (hw->running)" when checking if a warning should be printed, in case the time left is truncated down to 0 seconds.

  drivers/watchdog/bcm63xx_wdt.c | 37 +++++++++++++++++++++++++++++++++++++
  1 file changed, 37 insertions(+)

drivers/watchdog/bcm63xx_wdt.c +36 -0

index 3c7667a..9d099e0 100644
@@ -14,6 +14,7 @@
  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
  #include <linux/clk.h>
+#include <linux/delay.h>
  #include <linux/errno.h>
  #include <linux/io.h>
  #include <linux/kernel.h>

@@ -75,6 +76,19 @@ static int bcm63xx_wdt_stop(struct watchdog_device *wdd)
  	return 0;
  }
 
+static unsigned int bcm63xx_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct bcm63xx_wdt_hw *hw = watchdog_get_drvdata(wdd);
+	unsigned long flags;
+	u32 val;
+
+	raw_spin_lock_irqsave(&hw->lock, flags);
+	val = __raw_readl(hw->regs + WDT_CTL_REG);
+	val /= hw->clock_hz;
+	raw_spin_unlock_irqrestore(&hw->lock, flags);
+	return val;
+}
+
  static int bcm63xx_wdt_set_timeout(struct watchdog_device *wdd,
  	unsigned int timeout)
  {

@@ -130,6 +144,7 @@ static struct watchdog_ops bcm63xx_wdt_ops = {
  	.owner = THIS_MODULE,
  	.start = bcm63xx_wdt_start,
  	.stop = bcm63xx_wdt_stop,
+	.get_timeleft = bcm63xx_wdt_get_timeleft,
  	.set_timeout = bcm63xx_wdt_set_timeout,
  };
 

@@ -144,6 +159,8 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
  	struct bcm63xx_wdt_hw *hw;
  	struct watchdog_device *wdd;
  	struct resource *r;
+	u32 timeleft1, timeleft2;
+	unsigned int timeleft;
  	int ret;
 
  	hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL);

@@ -197,6 +214,23 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
  	watchdog_init_timeout(wdd, 0, &pdev->dev);
  	watchdog_set_nowayout(wdd, nowayout);
 
+	/* Compare two reads of the time left value, 2 clock ticks apart */
+	rmb();
+	timeleft1 = __raw_readl(hw->regs + WDT_CTL_REG);
+	udelay(DIV_ROUND_UP(1000000, hw->clock_hz / 2));
+	/* Ensure the register is read twice */
+	rmb();
+	timeleft2 = __raw_readl(hw->regs + WDT_CTL_REG);
+
+	/* If the time left is changing, the watchdog is running */
+	if (timeleft1 != timeleft2) {
+		hw->running = true;
+		timeleft = bcm63xx_wdt_get_timeleft(wdd);
+	} else {
+		hw->running = false;
+		timeleft = 0;
+	}
+
  	ret = bcm63xx_timer_register(TIMER_WDT_ID, bcm63xx_wdt_isr, wdd);
  	if (ret < 0) {
  		dev_err(&pdev->dev, "failed to register wdt timer isr\n");

@@ -214,6 +248,8 @@ static int bcm63xx_wdt_probe(struct platform_device *pdev)
  		dev_name(wdd->dev), hw->regs,
  		wdd->timeout, wdd->max_timeout);
 
+	if (hw->running)
+		dev_alert(wdd->dev, "running, reboot in %us\n", timeleft);
  	return 0;
 
  unregister_timer:

Quoted 4 lines · Simon Arlott

Return the remaining time from the hardware control register.

Warn when the device is registered if the hardware watchdog is currently running and report the remaining time left.

This is really two logical changes, isn't it ?

Nice trick to figure out if the watchdog is running.

What is the impact ? Will this result in interrupts ?

If so, would it make sense to _not_ reset the system after a timeout in this case, but to keep pinging the watchdog while the watchdog device is not open ?

This is really two logical changes, isn't it ?

This is really two logical changes, isn't it ?

This is really two logical changes, isn't it ?