[PATCH v10 0/5] Watchdog: introduce ARM SBSA watchdog driver

From: Fu Wei <redacted>

This patchset:
    (1)Introduce Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt
    for FDT info of SBSA Generic Watchdog, and give two examples of
    adding SBSA Generic Watchdog device node into the dts files:
    foundation-v8.dts and amd-seattle-soc.dtsi.

    (2)Introduce ARM SBSA watchdog driver:
        a.Use linux kernel watchdog framework;
        b.Work with FDT on ARM64;
        c.Support getting timeout from parameter and FDT at the driver
          init stage.
        d.Separate the driver to two parts:
          (1) single stage timeout driver(ignore WS0 interrupt)
          (2) register WS0 interrupt for the half timeout panic
        e.Timeout is from watchdog enabled to WS1 triggered.
          User can disable "half timeout panic" by kernel config or
          module parameter panic_enabled.

Changelog:
v10:Delete pretimeout support
    Separate the driver to two parts:
        (1) single stage timeout driver(ignore WS0 interrupt)
        (2) register WS0 interrupt for the half timeout panic
    timeout == (enable --> WS1)

v9: https://lkml.org/lkml/2015/11/9/57
    Rebase to latest kernel version(4.3).
    Update the Documentation of sbsa-gwdt device node info of FDT:
        (1) move some introduction to pretimeout patch
        (2) delete WS1 value from "interrupts" of binding documentation,
            since WS1 won't be handled by Linux.

v8: https://lkml.org/lkml/2015/10/27/466
    Rebase to latest kernel version(4.3-rc7).
    Separate the patches of GTDT support and arm_arch_timer. This
    clocksource relevant patch will upstreamed in a individual patchset.
    Update all the default timeout and pretimeout to 30s and 60s.
    Improve documentation and inline comments.
    Fix a bug in pretimeout support which makes timeout and pretimeout
    parameters initialization fail.

v7: https://lkml.org/lkml/2015/8/24/611
    Rebase to latest kernel version(4.2-rc7).
    Improve FDT support: geting resource by order, instead of name.
    According to the FDT support, Update the example dts file, gtdt.c
    and sbsa_gwdt.c.
    Pass the sparse test, and fix the warning.
    Fix the max_pretimeout and max_timeout value overflow bug.
    Delete the WCV output value.
    

v6: https://lkml.org/lkml/2015/6/23/359
    Improve the dtb example files: reduce the register frame size to 4K.
    Improve pretimeout support:
        (1) improve watchdog_init_timeouts function
	(2) rename watchdog_check_min_max_timeouts back to the original name
        (1) improve watchdog_timeout_invalid/watchdog_pretimeout_invalid
    Add the new features in the sbsa_gwdt driver:
	(1) In the second stage, user can feed the dog without cleaning WS0.
	(2) In the second stage, user can trigger WS1 by setting pretimeout = 0.
	(3) expand the max value of pretimeout, in case 10 second is not enough
	    for a kdump kernel reboot in panic.

v5: https://lkml.org/lkml/2015/6/10/357
    Improve pretimeout support:
        (1)fix typo in documentation and comments.
	(2)fix the timeout limits validation bug.
    Simplify sbsa_gwdt driver:
	(1)integrate all the registers access functions into caller.

v4: https://lkml.org/lkml/2015/6/2/4
    Refactor GTDT support code: remove it from arch/arm64/kernel/acpi.c,
    put it into drivers/acpi/gtdt.c file.
    Integrate the GTDT code of drivers/clocksource/arm_arch_timer.c into
    drivers/acpi/gtdt.c.
    Improve pretimeout support, fix "pretimeout == 0" problem.
    Simplify sbsa_gwdt driver:
        (1)timeout/pretimeout limits setup;
        (2)keepalive function;
        (3)delete "clk == 0" check;
        (4)delete WS0 status bit check in interrupt routine;
        (5)sbsa_gwdt_set_wcv function.

v3: https://lkml.org/lkml/2015/5/25/111
    Delete "export arch_timer_get_rate" patch.
    Driver back to use arch_timer_get_cntfrq.
    Improve watchdog_init_timeouts function and update relevant documentation.
    Improve watchdog_timeout_invalid and watchdog_pretimeout_invalid.
    Improve foundation-v8.dts: delete the unnecessary tag of device node.
    Remove "ARM64 || COMPILE_TEST" from Kconfig.
    Add comments in arch/arm64/kernel/acpi.c
    Fix typoes and incorrect comments.

v2: https://lkml.org/lkml/2015/5/21/172
    Improve watchdog-kernel-api.txt documentation for pretimeout support.
    Export "arch_timer_get_rate" in arm_arch_timer.c.
    Add watchdog_init_timeouts API for pretimeout support in framework.
    Improve suspend and resume foundation in driver
    Improve timeout/pretimeout values init code in driver.
    Delete unnecessary items of the sbsa_gwdt struct and #define.
    Delete all unnecessary debug info in driver.
    Fix 64bit division bug.
    Use the arch_timer interface to get watchdog clock rate.
    Add MODULE_DEVICE_TABLE for platform device id.
    Fix typoes.

v1: https://lkml.org/lkml/2015/5/15/279
    The first version upstream patchset to linux mailing list.

Fu Wei (5):
  Documentation: add sbsa-gwdt driver documentation
  ARM64: add SBSA Generic Watchdog device node in foundation-v8.dts
  ARM64: add SBSA Generic Watchdog device node in amd-seattle-soc.dtsi
  Watchdog: introduce ARM SBSA watchdog driver
  Watchdog: ARM SBSA Generic Watchdog half timeout panic support

 .../devicetree/bindings/watchdog/sbsa-gwdt.txt     |  35 ++
 Documentation/watchdog/watchdog-parameters.txt     |   6 +
 arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi       |   9 +
 arch/arm64/boot/dts/arm/foundation-v8.dts          |   8 +
 drivers/watchdog/Kconfig                           |  27 ++
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/sbsa_gwdt.c                       | 366 +++++++++++++++++++++
 7 files changed, 452 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt
 create mode 100644 drivers/watchdog/sbsa_gwdt.c

-- 
2.5.0

From: Fu Wei <redacted>

The sbsa-gwdt.txt documentation in devicetree/bindings/watchdog is for
introducing SBSA(Server Base System Architecture) Generic Watchdog
device node info into FDT.

Also add sbsa-gwdt introduction in watchdog-parameters.txt

Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Fu Wei <redacted>
---
 .../devicetree/bindings/watchdog/sbsa-gwdt.txt     | 35 ++++++++++++++++++++++
 Documentation/watchdog/watchdog-parameters.txt     |  5 ++++
 2 files changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt
new file mode 100644
index 0000000..73a5d0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt

@@ -0,0 +1,35 @@
+* SBSA (Server Base System Architecture) Generic Watchdog
+
+The SBSA Generic Watchdog Timer is used to force a reset of the system
+after two stages of timeout have elapsed.  A detailed definition of the
+watchdog timer can be found in the ARM document: ARM-DEN-0029 - Server
+Base System Architecture (SBSA)
+
+Required properties:
+- compatible: Should at least contain "arm,sbsa-gwdt".
+
+- reg: Each entry specifies the base physical address of a register frame
+  and the length of that frame; currently, two frames must be defined,
+  in this order:
+  1: Watchdog control frame;
+  2: Refresh frame.
+
+- interrupts: Should contain the Watchdog Signal 0 (WS0) SPI (Shared
+  Peripheral Interrupt) number of SBSA Generic Watchdog.
+
+Optional properties
+- timeout-sec: Watchdog timeout values (in seconds).
+
+- status: Indicates the device is not available for use. Should be "okay"
+  or "disabled" for available/unavailable. Default is "okay".
+
+Example for FVP Foundation Model v8:
+
+watchdog at 2a440000 {
+	compatible = "arm,sbsa-gwdt";
+	reg = <0x0 0x2a440000 0 0x1000>,
+	      <0x0 0x2a450000 0 0x1000>;
+	interrupts = <0 27 4>;
+	timeout-sec = <30>;
+	status = "okay";
+};

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 9f9ec9f..300eb4d 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt

@@ -284,6 +284,11 @@ sbc_fitpc2_wdt:
 margin: Watchdog margin in seconds (default 60s)
 nowayout: Watchdog cannot be stopped once started
 -------------------------------------------------
+sbsa_gwdt:
+timeout: Watchdog timeout in seconds. (default 20s)
+nowayout: Watchdog cannot be stopped once started
+	(default=kernel config parameter)
+-------------------------------------------------
 sc1200wdt:
 isapnp: When set to 0 driver ISA PnP support will be disabled (default=1)
 io: io port

-- 
2.5.0

From: Fu Wei <redacted>

This can be a example of adding SBSA Generic Watchdog device node
into some dts files for the Soc which contains SBSA Generic Watchdog.

Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Fu Wei <redacted>
---
 arch/arm64/boot/dts/arm/foundation-v8.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/foundation-v8.dts b/arch/arm64/boot/dts/arm/foundation-v8.dts
index 4eac8dc..75da16b 100644
--- a/arch/arm64/boot/dts/arm/foundation-v8.dts
+++ b/arch/arm64/boot/dts/arm/foundation-v8.dts

@@ -237,4 +237,12 @@
 			};
 		};
 	};
+	watchdog at 2a440000 {
+		compatible = "arm,sbsa-gwdt";
+		reg = <0x0 0x2a440000 0 0x1000>,
+			<0x0 0x2a450000 0 0x1000>;
+		interrupts = <0 27 4>;
+		timeout-sec = <30>;
+		status = "okay";
+	};
 };

-- 
2.5.0

From: Fu Wei <redacted>

This can be a example of adding SBSA Generic Watchdog device node
into some dts files for the Soc which contains SBSA Generic Watchdog.

Acked-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Signed-off-by: Fu Wei <redacted>
---
 arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi b/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi
index 2874d92..057e9c0 100644
--- a/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi
+++ b/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi

@@ -84,6 +84,15 @@
 			clock-names = "uartclk", "apb_pclk";
 		};
 
+		watchdog0: watchdog at e0bb0000 {
+			compatible = "arm,sbsa-gwdt";
+			reg = <0x0 0xe0bc0000 0 0x1000>,
+				<0x0 0xe0bb0000 0 0x1000>;
+			interrupts = <0 337 4>;
+			timeout-sec = <30>;
+			status = "disabled";
+		};
+
 		spi0: ssp at e1020000 {
 			status = "disabled";
 			compatible = "arm,pl022", "arm,primecell";

-- 
2.5.0

From: Fu Wei <redacted>

According to Server Base System Architecture (SBSA) specification,
the SBSA Generic Watchdog has two stage timeouts: the first signal (WS0)
is for alerting the system by interrupt, the second one (WS1) is a real
hardware reset.

This patch initially implements a simple single stage watchdog driver:
when the timeout is reached, your system will be reset by the second
signal (WS1).
The first signal (WS0) is ignored in this driver.

This driver bases on linux kernel watchdog framework, so it can get
timeout from module parameter and FDT at the driver init stage.

Signed-off-by: Fu Wei <redacted>
Reviewed-by: Graeme Gregory <redacted>
Tested-by: Pratyush Anand <redacted>
---
 drivers/watchdog/Kconfig     |  17 +++
 drivers/watchdog/Makefile    |   1 +
 drivers/watchdog/sbsa_gwdt.c | 322 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 340 insertions(+)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4f0e7be..4ab1b05 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig

@@ -201,6 +201,23 @@ config ARM_SP805_WATCHDOG
 	  ARM Primecell SP805 Watchdog timer. This will reboot your system when
 	  the timeout is reached.
 
+config ARM_SBSA_WATCHDOG
+	tristate "ARM SBSA Generic Watchdog"
+	depends on ARM64
+	depends on ARM_ARCH_TIMER
+	select WATCHDOG_CORE
+	help
+	  ARM SBSA Generic Watchdog has two stage timeouts:
+	  the first signal (WS0) is for alerting the system by interrupt,
+	  the second one (WS1) is a real hardware reset.
+	  More details: ARM DEN0029B - Server Base System Architecture (SBSA)
+
+	  This is a simple single stage driver: when the timeout is reached,
+	  your system will be reset by WS1. The first signal (WS0) is ignored.
+
+	  To compile this driver as module, choose M here: The module
+	  will be called sbsa_gwdt.
+
 config ASM9260_WATCHDOG
 	tristate "Alphascale ASM9260 watchdog"
 	depends on MACH_ASM9260

diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f566753..f9826d4 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile

@@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
 
 # ARM Architecture
 obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
+obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o
 obj-$(CONFIG_ASM9260_WATCHDOG) += asm9260_wdt.o
 obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o
 obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
new file mode 100644
index 0000000..5a2dba3
--- /dev/null
+++ b/drivers/watchdog/sbsa_gwdt.c

@@ -0,0 +1,322 @@
+/*
+ * SBSA(Server Base System Architecture) Generic Watchdog driver
+ *
+ * Copyright (c) 2015, Linaro Ltd.
+ * Author: Fu Wei <fu.wei@linaro.org>
+ *         Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
+ *         Al Stone <al.stone@linaro.org>
+ *         Timur Tabi <timur@codeaurora.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This SBSA Generic watchdog driver is a single stage timeout version.
+ * Since this watchdog timer has two stages, and each stage is determined
+ * by WOR. So the timeout is (WOR * 2).
+ * When first timeout is reached, WS0 is triggered, the interrupt
+ * triggered by WS0 will be ignored, then the second watch period starts;
+ * when second timeout is reached, then WS1 is triggered, system reset.
+ *
+ * More details about the hardware specification of this device:
+ * ARM DEN0029B - Server Base System Architecture (SBSA)
+ *
+ * SBSA GWDT: |--------WOR-------WS0--------WOR-------WS1
+ *            |----------------timeout----------------reset
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/uaccess.h>
+#include <linux/watchdog.h>
+#include <asm/arch_timer.h>
+
+/* SBSA Generic Watchdog register definitions */
+/* refresh frame */
+#define SBSA_GWDT_WRR				0x000
+
+/* control frame */
+#define SBSA_GWDT_WCS				0x000
+#define SBSA_GWDT_WOR				0x008
+#define SBSA_GWDT_WCV				0x010
+
+/* refresh/control frame */
+#define SBSA_GWDT_W_IIDR			0xfcc
+#define SBSA_GWDT_IDR				0xfd0
+
+/* Watchdog Control and Status Register */
+#define SBSA_GWDT_WCS_EN			BIT(0)
+#define SBSA_GWDT_WCS_WS0			BIT(1)
+#define SBSA_GWDT_WCS_WS1			BIT(2)
+
+/**
+ * struct sbsa_gwdt - Internal representation of the SBSA GWDT
+ * @wdd:		kernel watchdog_device structure
+ * @clk:		store the System Counter clock frequency, in Hz.
+ * @refresh_base:	Virtual address of the watchdog refresh frame
+ * @control_base:	Virtual address of the watchdog control frame
+ */
+struct sbsa_gwdt {
+	struct watchdog_device	wdd;
+	u32			clk;
+	void __iomem		*refresh_base;
+	void __iomem		*control_base;
+};
+
+#define to_sbsa_gwdt(e) container_of(e, struct sbsa_gwdt, wdd)
+
+#define DEFAULT_TIMEOUT		20 /* seconds, the 1st + 2nd watch periods*/
+
+static unsigned int timeout;
+module_param(timeout, uint, 0);
+MODULE_PARM_DESC(timeout,
+		 "Watchdog timeout in seconds. (>=0, default="
+		 __MODULE_STRING(DEFAULT_TIMEOUT) ")");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, S_IRUGO);
+MODULE_PARM_DESC(nowayout,
+		 "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+/*
+ * watchdog operation functions
+ */
+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
+				 unsigned int timeout)
+{
+	struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+	wdd->timeout = timeout;
+	writel(timeout / 2 * gwdt->clk, gwdt->control_base + SBSA_GWDT_WOR);
+
+	return 0;
+}
+
+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+	u64 timeleft = readq(gwdt->control_base + SBSA_GWDT_WCV) -
+		       arch_counter_get_cntvct();
+
+	do_div(timeleft, gwdt->clk);
+
+	if (readl(gwdt->control_base + SBSA_GWDT_WCS) & SBSA_GWDT_WCS_WS0)
+		return timeleft;
+
+	return timeleft + wdd->timeout / 2;
+}
+
+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
+{
+	struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+	/*
+	* Writing WRR for an explicit watchdog refresh.
+	* You can write anyting(like 0xc0ffee).
+	*/
+	writel(0xc0ffee, gwdt->refresh_base + SBSA_GWDT_WRR);
+
+	return 0;
+}
+
+static unsigned int sbsa_gwdt_status(struct watchdog_device *wdd)
+{
+	struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+	u32 status = readl(gwdt->control_base + SBSA_GWDT_WCS);
+
+	/* is the watchdog timer running? */
+	return (status & SBSA_GWDT_WCS_EN) << WDOG_ACTIVE;
+}
+
+static int sbsa_gwdt_start(struct watchdog_device *wdd)
+{
+	struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+	/* writing WCS will cause an explicit watchdog refresh */
+	writel(SBSA_GWDT_WCS_EN, gwdt->control_base + SBSA_GWDT_WCS);
+
+	return sbsa_gwdt_keepalive(wdd);
+}
+
+static int sbsa_gwdt_stop(struct watchdog_device *wdd)
+{
+	struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+	writel(0, gwdt->control_base + SBSA_GWDT_WCS);
+
+	return 0;
+}
+
+static struct watchdog_info sbsa_gwdt_info = {
+	.identity	= "SBSA Generic Watchdog",
+	.options	= WDIOF_SETTIMEOUT |
+			  WDIOF_KEEPALIVEPING |
+			  WDIOF_MAGICCLOSE |
+			  WDIOF_CARDRESET,
+};
+
+static struct watchdog_ops sbsa_gwdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= sbsa_gwdt_start,
+	.stop		= sbsa_gwdt_stop,
+	.status		= sbsa_gwdt_status,
+	.ping		= sbsa_gwdt_keepalive,
+	.set_timeout	= sbsa_gwdt_set_timeout,
+	.get_timeleft	= sbsa_gwdt_get_timeleft,
+};
+
+static int sbsa_gwdt_probe(struct platform_device *pdev)
+{
+	void __iomem *rf_base, *cf_base;
+	struct device *dev = &pdev->dev;
+	struct watchdog_device *wdd;
+	struct sbsa_gwdt *gwdt;
+	struct resource *res;
+	u32 status;
+	int ret;
+
+	gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
+	if (!gwdt)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, gwdt);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cf_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(cf_base))
+		return PTR_ERR(cf_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	rf_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(rf_base))
+		return PTR_ERR(rf_base);
+
+	/*
+	 * Get the frequency of system counter from the cp15 interface of ARM
+	 * Generic timer. We don't need to check it, because if it returns "0",
+	 * system would panic in very early stage.
+	 */
+	gwdt->clk = arch_timer_get_cntfrq();
+	gwdt->refresh_base = rf_base;
+	gwdt->control_base = cf_base;
+
+	wdd = &gwdt->wdd;
+	wdd->parent = dev;
+	wdd->info = &sbsa_gwdt_info;
+	wdd->ops = &sbsa_gwdt_ops;
+	watchdog_set_drvdata(wdd, gwdt);
+	watchdog_set_nowayout(wdd, nowayout);
+
+	wdd->min_timeout = 2;
+	wdd->max_timeout = U32_MAX / gwdt->clk * 2;
+	wdd->timeout = DEFAULT_TIMEOUT;
+	watchdog_init_timeout(wdd, timeout, dev);
+
+	status = readl(gwdt->control_base + SBSA_GWDT_WCS);
+	if (status & SBSA_GWDT_WCS_WS1) {
+		dev_warn(dev, "System reset by WDT.\n");
+		wdd->bootstatus |= WDIOF_CARDRESET;
+	}
+
+	ret = watchdog_register_device(wdd);
+	if (ret)
+		return ret;
+
+	/*
+	 * Update timeout to WOR.
+	 * Because of the explicit watchdog refresh mechanism,
+	 * it's also a ping, if watchdog is enabled.
+	 */
+	sbsa_gwdt_set_timeout(wdd, wdd->timeout);
+
+	dev_info(dev, "Initialized with %ds timeout @ %u Hz%s\n", wdd->timeout,
+		 gwdt->clk, status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
+
+	return 0;
+}
+
+static void sbsa_gwdt_shutdown(struct platform_device *pdev)
+{
+	struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev);
+
+	sbsa_gwdt_stop(&gwdt->wdd);
+}
+
+static int sbsa_gwdt_remove(struct platform_device *pdev)
+{
+	struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&gwdt->wdd);
+
+	return 0;
+}
+
+/* Disable watchdog if it is active during suspend */
+static int __maybe_unused sbsa_gwdt_suspend(struct device *dev)
+{
+	struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&gwdt->wdd))
+		sbsa_gwdt_stop(&gwdt->wdd);
+
+	return 0;
+}
+
+/* Enable watchdog and configure it if necessary */
+static int __maybe_unused sbsa_gwdt_resume(struct device *dev)
+{
+	struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&gwdt->wdd))
+		sbsa_gwdt_start(&gwdt->wdd);
+
+	return 0;
+}
+
+static const struct dev_pm_ops sbsa_gwdt_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume)
+};
+
+static const struct of_device_id sbsa_gwdt_of_match[] = {
+	{ .compatible = "arm,sbsa-gwdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
+
+static const struct platform_device_id sbsa_gwdt_pdev_match[] = {
+	{ .name = "sbsa-gwdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match);
+
+static struct platform_driver sbsa_gwdt_driver = {
+	.driver = {
+		.name = "sbsa-gwdt",
+		.pm = &sbsa_gwdt_pm_ops,
+		.of_match_table = sbsa_gwdt_of_match,
+	},
+	.probe = sbsa_gwdt_probe,
+	.remove = sbsa_gwdt_remove,
+	.shutdown = sbsa_gwdt_shutdown,
+	.id_table = sbsa_gwdt_pdev_match,
+};
+
+module_platform_driver(sbsa_gwdt_driver);
+
+MODULE_DESCRIPTION("SBSA Generic Watchdog Driver");
+MODULE_AUTHOR("Fu Wei <fu.wei@linaro.org>");
+MODULE_AUTHOR("Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>");
+MODULE_AUTHOR("Al Stone <al.stone@linaro.org>");
+MODULE_AUTHOR("Timur Tabi <timur@codeaurora.org>");
+MODULE_LICENSE("GPL v2");

-- 
2.5.0

From: Fu Wei <redacted>

This patch registers the WS0 interrupt routine to trigger panic,
when the watchdog reachs the first stage (the half timeout).
This function can help administrator to backup the system context
info by panic console output or kdump (if supported), once system
goes wrong (doesn't feed the watchdog in the half timeout).

User also can skip panic by setting panic_enabled (module parameter) as 0

Signed-off-by: Fu Wei <redacted>
---
 Documentation/watchdog/watchdog-parameters.txt |  1 +
 drivers/watchdog/Kconfig                       | 10 +++++
 drivers/watchdog/sbsa_gwdt.c                   | 54 +++++++++++++++++++++++---
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 300eb4d..31641e2 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt

@@ -286,6 +286,7 @@ nowayout: Watchdog cannot be stopped once started
 -------------------------------------------------
 sbsa_gwdt:
 timeout: Watchdog timeout in seconds. (default 20s)
+panic_enabled: Enable panic at half timeout. (default=true)
 nowayout: Watchdog cannot be stopped once started
 	(default=kernel config parameter)
 -------------------------------------------------

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4ab1b05..42adfdf 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig

@@ -218,6 +218,16 @@ config ARM_SBSA_WATCHDOG
 	  To compile this driver as module, choose M here: The module
 	  will be called sbsa_gwdt.
 
+config ARM_SBSA_WATCHDOG_PANIC
+	bool "ARM SBSA Generic Watchdog triggers panic at the half timeout"
+	depends on ARM_SBSA_WATCHDOG
+	help
+	  ARM SBSA Generic Watchdog will trigger panic in the first signal
+	  (WS0) interrupt routine when the half timeout is reached.
+	  This function can help administrator to backup the system context
+	  info by panic console output or kdump (if supported).
+	  But user can skip panic by setting moduleparam panic_enabled as 0.
+
 config ASM9260_WATCHDOG
 	tristate "Alphascale ASM9260 watchdog"
 	depends on MACH_ASM9260

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index 5a2dba3..d18cf37 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c

@@ -16,18 +16,22 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
- * This SBSA Generic watchdog driver is a single stage timeout version.
+ * This SBSA Generic watchdog driver is a two stages version.
  * Since this watchdog timer has two stages, and each stage is determined
  * by WOR. So the timeout is (WOR * 2).
- * When first timeout is reached, WS0 is triggered, the interrupt
- * triggered by WS0 will be ignored, then the second watch period starts;
- * when second timeout is reached, then WS1 is triggered, system reset.
+ * When the first stage(the half timeout) is reached, WS0 interrupt is
+ * triggered, at this moment the second watch period starts;
+ * In the WS0 interrupt routine, panic will be triggered for saving the
+ * system context.
+ * If the system is getting into trouble and cannot be reset by panic or
+ * restart properly by the kdump kernel(if supported), then the second
+ * stage (the timeout) will be reached, system will be reset by WS1.
  *
  * More details about the hardware specification of this device:
  * ARM DEN0029B - Server Base System Architecture (SBSA)
  *
  * SBSA GWDT: |--------WOR-------WS0--------WOR-------WS1
- *            |----------------timeout----------------reset
+ *            |--half_timeout--(panic)--half_timeout--reset
  *
  */
 

@@ -84,6 +88,13 @@ MODULE_PARM_DESC(timeout,
 		 "Watchdog timeout in seconds. (>=0, default="
 		 __MODULE_STRING(DEFAULT_TIMEOUT) ")");
 
+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+static bool panic_enabled = true;
+module_param(panic_enabled, bool, 0);
+MODULE_PARM_DESC(panic_enabled,
+		 "enable panic at half timeout. (default=true)");
+#endif
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, S_IRUGO);
 MODULE_PARM_DESC(nowayout,

@@ -159,6 +170,16 @@ static int sbsa_gwdt_stop(struct watchdog_device *wdd)
 	return 0;
 }
 
+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
+{
+	if (panic_enabled)
+		panic("SBSA Watchdog half timeout");
+
+	return IRQ_HANDLED;
+}
+#endif
+
 static struct watchdog_info sbsa_gwdt_info = {
 	.identity	= "SBSA Generic Watchdog",
 	.options	= WDIOF_SETTIMEOUT |

@@ -186,6 +207,9 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	struct resource *res;
 	u32 status;
 	int ret;
+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+	int irq;
+#endif
 
 	gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
 	if (!gwdt)

@@ -202,6 +226,14 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 	if (IS_ERR(rf_base))
 		return PTR_ERR(rf_base);
 
+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "unable to get ws0 interrupt.\n");
+		return irq;
+	}
+#endif
+
 	/*
 	 * Get the frequency of system counter from the cp15 interface of ARM
 	 * Generic timer. We don't need to check it, because if it returns "0",

@@ -228,6 +260,14 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 		dev_warn(dev, "System reset by WDT.\n");
 		wdd->bootstatus |= WDIOF_CARDRESET;
 	}
+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+	ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
+			       pdev->name, gwdt);
+	if (ret) {
+		dev_err(dev, "unable to request IRQ %d\n", irq);
+		return ret;
+	}
+#endif
 
 	ret = watchdog_register_device(wdd);
 	if (ret)

@@ -242,6 +282,10 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
 
 	dev_info(dev, "Initialized with %ds timeout @ %u Hz%s\n", wdd->timeout,
 		 gwdt->clk, status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+	dev_info(dev, "Half timeout panic %s.\n",
+		 panic_enabled ? "enabled" : "disabled");
+#endif
 
 	return 0;
 }

-- 
2.5.0

fu.wei at linaro.org wrote:

+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+static bool panic_enabled = true;

I think this should default to 'false', because IMHO, this seems like an 
odd feature.  I'm not crazy about the fact that there's a Kconfig option 
for it either, but I'm not going to NACK this patch.

I personally would prefer to drop this patch, and just wait for 
full-blown pre-timeout support.  It feels like a debugging feature that 
doesn't really belong upstream.  But like I said, it's just my opinion, 
and I won't complain if I'm outvoted.

fu.wei at linaro.org wrote:

+static struct platform_driver sbsa_gwdt_driver = {
+	.driver = {
+		.name = "sbsa-gwdt",
+		.pm = &sbsa_gwdt_pm_ops,
+		.of_match_table = sbsa_gwdt_of_match,
+	},
+	.probe = sbsa_gwdt_probe,
+	.remove = sbsa_gwdt_remove,
+	.shutdown = sbsa_gwdt_shutdown,
+	.id_table = sbsa_gwdt_pdev_match,
+};

I just noticed you dropped ACPI support.  Server platforms are supposed 
to use ACPI, so that seems like a critical omission to me.  You had a 
GTDT parser in an older version of this patch.  What happened to it?

Hi Timur,

Thanks for your rapid feedback :-)

On 4 February 2016 at 01:27, Timur Tabi [off-list ref] wrote:

fu.wei at linaro.org wrote:

quoted

+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+static bool panic_enabled = true;

I think this should default to 'false', because IMHO, this seems like an odd

yes, It make sense to make it default to 'false'.

feature.  I'm not crazy about the fact that there's a Kconfig option for it
either, but I'm not going to NACK this patch.

I personally would prefer to drop this patch, and just wait for full-blown
pre-timeout support.  It feels like a debugging feature that doesn't really

sorry, are you saying : using pre-timeout instead of this half timeout?

But even we have pre-timeout support, pre-timeout  == timeout / 2, it
can not be configured without touch timeout.

if you want pre-timeout  != timeout / 2, we have to modify WCV in the
interrupt routine.
 (because of the explicit watchdog refresh  mechanism)

Could you let me know why we need pre-timeout  here ?? :-)

belong upstream.  But like I said, it's just my opinion, and I won't
complain if I'm outvoted.

 I think this debugging feature is the  purpose of the two-stage
watchdog, if I understand correctly



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021

Fu Wei wrote:

sorry, are you saying : using pre-timeout instead of this half timeout?

But even we have pre-timeout support, pre-timeout  == timeout / 2, it
can not be configured without touch timeout.

if you want pre-timeout  != timeout / 2, we have to modify WCV in the
interrupt routine.
  (because of the explicit watchdog refresh  mechanism)

Could you let me know why we need pre-timeout  here ??:-)

What I meant was that if we had full-blown pre-timeout support in the 
watchdog layer, then you could use that to implement the 
panic-on-half-timeout feature.

When pre-timeout is implemented, will you modify the interrupt handler 
to use it?

quoted

quoted

belong upstream.  But like I said, it's just my opinion, and I won't
complain if I'm outvoted.

  I think this debugging feature is the  purpose of the two-stage
watchdog, if I understand correctly

Hmmm... that make sense.  I think maybe you should drop the Kconfig 
option, and just have "static bool panic_enabled = false;"  Also, then 
do this:

if (panic_enabled) {
	ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
			       pdev->name, gwdt);
	if (ret) {
		dev_err(dev, "unable to request IRQ %d\n", irq);
		return ret;
	}
}

That way, the interrupt handler is never registered if the command-line 
parameter is not specified.

Hi Timur,

On 4 February 2016 at 01:48, Timur Tabi [off-list ref] wrote:

fu.wei at linaro.org wrote:

quoted

+static struct platform_driver sbsa_gwdt_driver = {
+       .driver = {
+               .name = "sbsa-gwdt",
+               .pm = &sbsa_gwdt_pm_ops,
+               .of_match_table = sbsa_gwdt_of_match,
+       },
+       .probe = sbsa_gwdt_probe,
+       .remove = sbsa_gwdt_remove,
+       .shutdown = sbsa_gwdt_shutdown,
+       .id_table = sbsa_gwdt_pdev_match,
+};

I just noticed you dropped ACPI support.  Server platforms are supposed to
use ACPI, so that seems like a critical omission to me.  You had a GTDT
parser in an older version of this patch.  What happened to it?

I have posted  GTDT support separately : https://lkml.org/lkml/2016/2/1/660

devicetree driver and GTDT driver both export sbsa gwdt info to
"platform resource".

this driver get hardware info from platform resource.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021

Fu Wei wrote:

I have posted  GTDT support separately :https://lkml.org/lkml/2016/2/1/660

devicetree driver and GTDT driver both export sbsa gwdt info to
"platform resource".

this driver get hardware info from platform resource.

I must be missing something.  How does the driver probe?  It only has an 
entry for device tree:

+		.of_match_table = sbsa_gwdt_of_match,

Doesn't there need to be an ACPI match table to probe on an ACPI system?

Hi Timur

On 4 February 2016 at 01:53, Timur Tabi [off-list ref] wrote:

Fu Wei wrote:

quoted

sorry, are you saying : using pre-timeout instead of this half timeout?

But even we have pre-timeout support, pre-timeout  == timeout / 2, it
can not be configured without touch timeout.

if you want pre-timeout  != timeout / 2, we have to modify WCV in the
interrupt routine.
  (because of the explicit watchdog refresh  mechanism)

Could you let me know why we need pre-timeout  here ??:-)

What I meant was that if we had full-blown pre-timeout support in the
watchdog layer, then you could use that to implement the
panic-on-half-timeout feature.

When pre-timeout is implemented, will you modify the interrupt handler to
use it?

Sorry I am little confused.

Actually I am taking your suggestion to avoid touching WCV in
interrupt routine.
So even we have pre-timeout support , it is useless for this
panic-on-half-timeout feature,
because pre-timeout  == timeout / 2 (always).

So maybe I misunderstand your suggestion,
could you let me know : why we want pre-timeout here?

quoted

quoted

quoted

belong upstream.  But like I said, it's just my opinion, and I won't
complain if I'm outvoted.

  I think this debugging feature is the  purpose of the two-stage
watchdog, if I understand correctly

Hmmm... that make sense.  I think maybe you should drop the Kconfig option,
and just have "static bool panic_enabled = false;"  Also, then do this:

if (panic_enabled) {
        ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, 0,
                               pdev->name, gwdt);
        if (ret) {
                dev_err(dev, "unable to request IRQ %d\n", irq);
                return ret;
        }
}

yes, agree

That way, the interrupt handler is never registered if the command-line
parameter is not specified.

-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021

Fu Wei wrote:

Actually I am taking your suggestion to avoid touching WCV in
interrupt routine.
So even we have pre-timeout support , it is useless for this
panic-on-half-timeout feature,
because pre-timeout  == timeout / 2 (always).

So maybe I misunderstand your suggestion,
could you let me know : why we want pre-timeout here?

Maybe I'm confused.

For pre-timeout, I think the SBSA watchdog driver should support only 
half-timeout.  That is, the user cannot configure the length of the 
pre-timeout with this driver.  He can only enable it, and it is 
automatically set to 1/2 timeout.

So when pre-timeout occurs, the interrupt handler calls panic() or 
whatever it's supposed to do.

So "pre-timeout  == timeout / 2 (always)" is exactly what we want.

On 4 February 2016 at 01:58, Timur Tabi [off-list ref] wrote:

Fu Wei wrote:

quoted

I have posted  GTDT support separately :https://lkml.org/lkml/2016/2/1/660

devicetree driver and GTDT driver both export sbsa gwdt info to
"platform resource".

this driver get hardware info from platform resource.

I must be missing something.  How does the driver probe?  It only has an

maybe you miss a  line of code for platform resource :

---------------------
static const struct platform_device_id sbsa_gwdt_pdev_match[] = {
 { .name = "sbsa-gwdt", },
 {},
};
MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match);
---------------------

entry for device tree:

+               .of_match_table = sbsa_gwdt_of_match,

Doesn't there need to be an ACPI match table to probe on an ACPI system?

No, we don't. And this have been tested.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021

Fu Wei wrote:

static const struct platform_device_id sbsa_gwdt_pdev_match[] = {
  { .name = "sbsa-gwdt", },
  {},
};
MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match);

Ok, thanks.

On 4 February 2016 at 02:08, Timur Tabi [off-list ref] wrote:

Fu Wei wrote:

quoted

Actually I am taking your suggestion to avoid touching WCV in
interrupt routine.
So even we have pre-timeout support , it is useless for this
panic-on-half-timeout feature,
because pre-timeout  == timeout / 2 (always).

So maybe I misunderstand your suggestion,
could you let me know : why we want pre-timeout here?

Maybe I'm confused.

For pre-timeout, I think the SBSA watchdog driver should support only
half-timeout.  That is, the user cannot configure the length of the
pre-timeout with this driver.  He can only enable it, and it is
automatically set to 1/2 timeout.

Actually,
the SBSA watchdog driver should support only  half-timeout for panic
the user cannot configure the length of "panic time",  He can only
enable it, and it is automatically set to 1/2 timeout.

we don't need pre-timeout here.

Hope I understand you correctly :-) sorry for your confusion

So when pre-timeout occurs, the interrupt handler calls panic() or whatever
it's supposed to do.

Actually,
So when 1/2 timeout occurs, the interrupt handler calls panic() or
whatever it's supposed to do.

So "pre-timeout  == timeout / 2 (always)" is exactly what we want.

our patchset is doing this way.



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021

Fu Wei wrote:

Actually,
the SBSA watchdog driver should support only  half-timeout for panic
the user cannot configure the length of "panic time",  He can only
enable it, and it is automatically set to 1/2 timeout.

we don't need pre-timeout here.

Hope I understand you correctly:-)  sorry for your confusion

quoted

quoted

So when pre-timeout occurs, the interrupt handler calls panic() or whatever
it's supposed to do.

Actually,
So when 1/2 timeout occurs, the interrupt handler calls panic() or
whatever it's supposed to do.

I understand all that.  What I'm saying is that, in the future, when 
pre-timeout support is added to the watchdog layer, I think it makes 
sense to modify this driver to use pre-timeout support.

On 4 February 2016 at 02:26, Timur Tabi [off-list ref] wrote:

Fu Wei wrote:

quoted

Actually,
the SBSA watchdog driver should support only  half-timeout for panic
the user cannot configure the length of "panic time",  He can only
enable it, and it is automatically set to 1/2 timeout.

we don't need pre-timeout here.

Hope I understand you correctly:-)  sorry for your confusion

quoted

quoted

So when pre-timeout occurs, the interrupt handler calls panic() or
whatever
it's supposed to do.

Actually,
So when 1/2 timeout occurs, the interrupt handler calls panic() or
whatever it's supposed to do.

I understand all that.  What I'm saying is that, in the future, when
pre-timeout support is added to the watchdog layer, I think it makes sense
to modify this driver to use pre-timeout support.

As you know I have made the pre-timeout support patch, If people like
it, i am happy to go on upstream it separately.

If we want to use pre-timeout here, user only can use get_pretimeout
and disable panic by setting pretimeout to 0
but user can not really set pretimeout, because "pre-timeout  ==
timeout / 2 (always)".
if user want to change pretimeout, he/she has to set_time instead.




-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021

Fu Wei wrote:

As you know I have made the pre-timeout support patch, If people like
it, i am happy to go on upstream it separately.

If we want to use pre-timeout here, user only can use get_pretimeout
and disable panic by setting pretimeout to 0
but user can not really set pretimeout, because "pre-timeout  ==
timeout / 2 (always)".
if user want to change pretimeout, he/she has to set_time instead.

Ok, I think patches 4 and 5 should be combined, and I think the Kconfig 
entry should be removed and just use panic_enabled.

On 4 February 2016 at 02:45, Timur Tabi [off-list ref] wrote:

Fu Wei wrote:

quoted

As you know I have made the pre-timeout support patch, If people like
it, i am happy to go on upstream it separately.

If we want to use pre-timeout here, user only can use get_pretimeout
and disable panic by setting pretimeout to 0
but user can not really set pretimeout, because "pre-timeout  ==
timeout / 2 (always)".
if user want to change pretimeout, he/she has to set_time instead.

Ok, I think patches 4 and 5 should be combined, and I think the Kconfig
entry should be removed and just use panic_enabled.

NP, will update this patchset like that ,  thanks :-)


-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021

On 02/03/2016 03:00 PM, Fu Wei wrote:

On 4 February 2016 at 02:45, Timur Tabi [off-list ref] wrote:

quoted

Fu Wei wrote:

quoted

As you know I have made the pre-timeout support patch, If people like
it, i am happy to go on upstream it separately.

If we want to use pre-timeout here, user only can use get_pretimeout
and disable panic by setting pretimeout to 0
but user can not really set pretimeout, because "pre-timeout  ==
timeout / 2 (always)".
if user want to change pretimeout, he/she has to set_time instead.

Ok, I think patches 4 and 5 should be combined, and I think the Kconfig
entry should be removed and just use panic_enabled.

Agreed.

NP, will update this patchset like that ,  thanks :-)

Also, if panic is enabled, the timeout needs to be adjusted accordingly
(to only panic after the entire timeout period has expired, not after
half of it). We can not panic the system after timeout / 2.

I am not too happy with the parameter name (panic_enabled). How about
"action", to match machzwd ?

Thanks,
Guenter

Guenter Roeck wrote:

Also, if panic is enabled, the timeout needs to be adjusted accordingly
(to only panic after the entire timeout period has expired, not after
half of it). We can not panic the system after timeout / 2.

It's a debugging feature, not an actual watchdog timeout panic.  That's 
why it's disabled by default.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

On 02/04/2016 05:48 AM, Timur Tabi wrote:

Guenter Roeck wrote:

quoted

Also, if panic is enabled, the timeout needs to be adjusted accordingly
(to only panic after the entire timeout period has expired, not after
half of it). We can not panic the system after timeout / 2.

It's a debugging feature, not an actual watchdog timeout panic.  That's why it's disabled by default.

"* When the first stage(the half timeout) is reached, WS0 interrupt is
  * triggered, at this moment the second watch period starts;
  * In the WS0 interrupt routine, panic will be triggered for saving the
  * system context.
  * If the system is getting into trouble and cannot be reset by panic or
  * restart properly by the kdump kernel(if supported), then the second
  * stage (the timeout) will be reached, system will be reset by WS1."

That doesn't sound like debugging to me.

Guenter

On 3 February 2016 at 10:18,  [off-list ref] wrote:

It would be great to have a comment whenever hard coded values are
used.  That way people understand why things were set this way.  The
same applies for the rest of this patch.

+}
+
+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
+{
+       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+       /*
+       * Writing WRR for an explicit watchdog refresh.
+       * You can write anyting(like 0xc0ffee).
+       */
+       writel(0xc0ffee, gwdt->refresh_base + SBSA_GWDT_WRR);
+
+       return 0;
+}
+
+static unsigned int sbsa_gwdt_status(struct watchdog_device *wdd)
+{
+       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+       u32 status = readl(gwdt->control_base + SBSA_GWDT_WCS);
+
+       /* is the watchdog timer running? */
+       return (status & SBSA_GWDT_WCS_EN) << WDOG_ACTIVE;
+}
+
+static int sbsa_gwdt_start(struct watchdog_device *wdd)
+{
+       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+       /* writing WCS will cause an explicit watchdog refresh */
+       writel(SBSA_GWDT_WCS_EN, gwdt->control_base + SBSA_GWDT_WCS);
+
+       return sbsa_gwdt_keepalive(wdd);
+}
+
+static int sbsa_gwdt_stop(struct watchdog_device *wdd)
+{
+       struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+       writel(0, gwdt->control_base + SBSA_GWDT_WCS);
+
+       return 0;
+}
+
+static struct watchdog_info sbsa_gwdt_info = {
+       .identity       = "SBSA Generic Watchdog",
+       .options        = WDIOF_SETTIMEOUT |
+                         WDIOF_KEEPALIVEPING |
+                         WDIOF_MAGICCLOSE |
+                         WDIOF_CARDRESET,
+};
+
+static struct watchdog_ops sbsa_gwdt_ops = {
+       .owner          = THIS_MODULE,
+       .start          = sbsa_gwdt_start,
+       .stop           = sbsa_gwdt_stop,
+       .status         = sbsa_gwdt_status,
+       .ping           = sbsa_gwdt_keepalive,
+       .set_timeout    = sbsa_gwdt_set_timeout,
+       .get_timeleft   = sbsa_gwdt_get_timeleft,
+};
+
+static int sbsa_gwdt_probe(struct platform_device *pdev)
+{
+       void __iomem *rf_base, *cf_base;
+       struct device *dev = &pdev->dev;
+       struct watchdog_device *wdd;
+       struct sbsa_gwdt *gwdt;
+       struct resource *res;
+       u32 status;
+       int ret;
+
+       gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
+       if (!gwdt)
+               return -ENOMEM;
+       platform_set_drvdata(pdev, gwdt);
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       cf_base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(cf_base))
+               return PTR_ERR(cf_base);
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+       rf_base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(rf_base))
+               return PTR_ERR(rf_base);
+
+       /*
+        * Get the frequency of system counter from the cp15 interface of ARM
+        * Generic timer. We don't need to check it, because if it returns "0",
+        * system would panic in very early stage.
+        */
+       gwdt->clk = arch_timer_get_cntfrq();
+       gwdt->refresh_base = rf_base;
+       gwdt->control_base = cf_base;
+
+       wdd = &gwdt->wdd;
+       wdd->parent = dev;
+       wdd->info = &sbsa_gwdt_info;
+       wdd->ops = &sbsa_gwdt_ops;
+       watchdog_set_drvdata(wdd, gwdt);
+       watchdog_set_nowayout(wdd, nowayout);
+
+       wdd->min_timeout = 2;
+       wdd->max_timeout = U32_MAX / gwdt->clk * 2;
+       wdd->timeout = DEFAULT_TIMEOUT;
+       watchdog_init_timeout(wdd, timeout, dev);
+
+       status = readl(gwdt->control_base + SBSA_GWDT_WCS);
+       if (status & SBSA_GWDT_WCS_WS1) {
+               dev_warn(dev, "System reset by WDT.\n");
+               wdd->bootstatus |= WDIOF_CARDRESET;
+       }
+
+       ret = watchdog_register_device(wdd);
+       if (ret)
+               return ret;
+
+       /*
+        * Update timeout to WOR.
+        * Because of the explicit watchdog refresh mechanism,
+        * it's also a ping, if watchdog is enabled.
+        */
+       sbsa_gwdt_set_timeout(wdd, wdd->timeout);
+
+       dev_info(dev, "Initialized with %ds timeout @ %u Hz%s\n", wdd->timeout,
+                gwdt->clk, status & SBSA_GWDT_WCS_EN ? " [enabled]" : "");
+
+       return 0;
+}
+
+static void sbsa_gwdt_shutdown(struct platform_device *pdev)
+{
+       struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev);
+
+       sbsa_gwdt_stop(&gwdt->wdd);
+}
+
+static int sbsa_gwdt_remove(struct platform_device *pdev)
+{
+       struct sbsa_gwdt *gwdt = platform_get_drvdata(pdev);
+
+       watchdog_unregister_device(&gwdt->wdd);
+
+       return 0;
+}
+
+/* Disable watchdog if it is active during suspend */
+static int __maybe_unused sbsa_gwdt_suspend(struct device *dev)
+{
+       struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
+
+       if (watchdog_active(&gwdt->wdd))
+               sbsa_gwdt_stop(&gwdt->wdd);
+
+       return 0;
+}
+
+/* Enable watchdog and configure it if necessary */
+static int __maybe_unused sbsa_gwdt_resume(struct device *dev)
+{
+       struct sbsa_gwdt *gwdt = dev_get_drvdata(dev);
+
+       if (watchdog_active(&gwdt->wdd))
+               sbsa_gwdt_start(&gwdt->wdd);
+
+       return 0;
+}
+
+static const struct dev_pm_ops sbsa_gwdt_pm_ops = {
+       SET_SYSTEM_SLEEP_PM_OPS(sbsa_gwdt_suspend, sbsa_gwdt_resume)
+};
+
+static const struct of_device_id sbsa_gwdt_of_match[] = {
+       { .compatible = "arm,sbsa-gwdt", },
+       {},
+};
+MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
+
+static const struct platform_device_id sbsa_gwdt_pdev_match[] = {
+       { .name = "sbsa-gwdt", },
+       {},
+};
+MODULE_DEVICE_TABLE(platform, sbsa_gwdt_pdev_match);
+
+static struct platform_driver sbsa_gwdt_driver = {
+       .driver = {
+               .name = "sbsa-gwdt",
+               .pm = &sbsa_gwdt_pm_ops,
+               .of_match_table = sbsa_gwdt_of_match,
+       },
+       .probe = sbsa_gwdt_probe,
+       .remove = sbsa_gwdt_remove,
+       .shutdown = sbsa_gwdt_shutdown,
+       .id_table = sbsa_gwdt_pdev_match,
+};
+
+module_platform_driver(sbsa_gwdt_driver);
+
+MODULE_DESCRIPTION("SBSA Generic Watchdog Driver");
+MODULE_AUTHOR("Fu Wei [off-list ref]");
+MODULE_AUTHOR("Suravee Suthikulpanit [off-list ref]");
+MODULE_AUTHOR("Al Stone [off-list ref]");
+MODULE_AUTHOR("Timur Tabi [off-list ref]");
+MODULE_LICENSE("GPL v2");
--
2.5.0

On Thu, Feb 04, 2016 at 01:18:42AM +0800, fu.wei at linaro.org wrote:

+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)
+{
+	struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+	/*
+	* Writing WRR for an explicit watchdog refresh.
+	* You can write anyting(like 0xc0ffee).
+	*/
+	writel(0xc0ffee, gwdt->refresh_base + SBSA_GWDT_WRR);
+
+	return 0;
+}

You might get in trouble for that. 0xd09f00d is probably less poisonous.

http://www.petpoisonhelpline.com/poison/caffeine/

Will

On 3 February 2016 at 10:18,  [off-list ref] wrote:

Can't the driver revert to single stage mode if platform_get_irq()
fails?  That way the value of 'irq' can be tested throughout the
_probe() function and the #ifdefs removed.

Thanks,
Mathieu

Will Deacon wrote:

quoted

+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd)

quoted

+{
+	struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
+
+	/*
+	* Writing WRR for an explicit watchdog refresh.
+	* You can write anyting(like 0xc0ffee).
+	*/
+	writel(0xc0ffee, gwdt->refresh_base + SBSA_GWDT_WRR);
+
+	return 0;
+}

You might get in trouble for that. 0xd09f00d is probably less poisonous.

http://www.petpoisonhelpline.com/poison/caffeine/

Any reason why we can't just keep it simple and write 0?

On 02/04/2016 08:32 AM, Mathieu Poirier wrote:

On 3 February 2016 at 10:18,  [off-list ref] wrote:

quoted

From: Fu Wei <redacted>

This patch registers the WS0 interrupt routine to trigger panic,
when the watchdog reachs the first stage (the half timeout).
This function can help administrator to backup the system context
info by panic console output or kdump (if supported), once system
goes wrong (doesn't feed the watchdog in the half timeout).

User also can skip panic by setting panic_enabled (module parameter) as 0

Signed-off-by: Fu Wei <redacted>
---
  Documentation/watchdog/watchdog-parameters.txt |  1 +
  drivers/watchdog/Kconfig                       | 10 +++++
  drivers/watchdog/sbsa_gwdt.c                   | 54 +++++++++++++++++++++++---
  3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/Documentation/watchdog/watchdog-parameters.txt b/Documentation/watchdog/watchdog-parameters.txt
index 300eb4d..31641e2 100644
--- a/Documentation/watchdog/watchdog-parameters.txt
+++ b/Documentation/watchdog/watchdog-parameters.txt

@@ -286,6 +286,7 @@ nowayout: Watchdog cannot be stopped once started
  -------------------------------------------------
  sbsa_gwdt:
  timeout: Watchdog timeout in seconds. (default 20s)
+panic_enabled: Enable panic at half timeout. (default=true)
  nowayout: Watchdog cannot be stopped once started
         (default=kernel config parameter)
  -------------------------------------------------

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4ab1b05..42adfdf 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig

@@ -218,6 +218,16 @@ config ARM_SBSA_WATCHDOG
           To compile this driver as module, choose M here: The module
           will be called sbsa_gwdt.

+config ARM_SBSA_WATCHDOG_PANIC
+       bool "ARM SBSA Generic Watchdog triggers panic at the half timeout"
+       depends on ARM_SBSA_WATCHDOG
+       help
+         ARM SBSA Generic Watchdog will trigger panic in the first signal
+         (WS0) interrupt routine when the half timeout is reached.
+         This function can help administrator to backup the system context
+         info by panic console output or kdump (if supported).
+         But user can skip panic by setting moduleparam panic_enabled as 0.
+
  config ASM9260_WATCHDOG
         tristate "Alphascale ASM9260 watchdog"
         depends on MACH_ASM9260

diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c
index 5a2dba3..d18cf37 100644
--- a/drivers/watchdog/sbsa_gwdt.c
+++ b/drivers/watchdog/sbsa_gwdt.c

@@ -16,18 +16,22 @@
   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   * GNU General Public License for more details.
   *
- * This SBSA Generic watchdog driver is a single stage timeout version.
+ * This SBSA Generic watchdog driver is a two stages version.
   * Since this watchdog timer has two stages, and each stage is determined
   * by WOR. So the timeout is (WOR * 2).
- * When first timeout is reached, WS0 is triggered, the interrupt
- * triggered by WS0 will be ignored, then the second watch period starts;
- * when second timeout is reached, then WS1 is triggered, system reset.
+ * When the first stage(the half timeout) is reached, WS0 interrupt is
+ * triggered, at this moment the second watch period starts;
+ * In the WS0 interrupt routine, panic will be triggered for saving the
+ * system context.
+ * If the system is getting into trouble and cannot be reset by panic or
+ * restart properly by the kdump kernel(if supported), then the second
+ * stage (the timeout) will be reached, system will be reset by WS1.
   *
   * More details about the hardware specification of this device:
   * ARM DEN0029B - Server Base System Architecture (SBSA)
   *
   * SBSA GWDT: |--------WOR-------WS0--------WOR-------WS1
- *            |----------------timeout----------------reset
+ *            |--half_timeout--(panic)--half_timeout--reset
   *
   */

@@ -84,6 +88,13 @@ MODULE_PARM_DESC(timeout,
                  "Watchdog timeout in seconds. (>=0, default="
                  __MODULE_STRING(DEFAULT_TIMEOUT) ")");

+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+static bool panic_enabled = true;
+module_param(panic_enabled, bool, 0);
+MODULE_PARM_DESC(panic_enabled,
+                "enable panic at half timeout. (default=true)");
+#endif
+
  static bool nowayout = WATCHDOG_NOWAYOUT;
  module_param(nowayout, bool, S_IRUGO);
  MODULE_PARM_DESC(nowayout,

@@ -159,6 +170,16 @@ static int sbsa_gwdt_stop(struct watchdog_device *wdd)
         return 0;
  }

+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id)
+{
+       if (panic_enabled)
+               panic("SBSA Watchdog half timeout");
+
+       return IRQ_HANDLED;
+}
+#endif
+
  static struct watchdog_info sbsa_gwdt_info = {
         .identity       = "SBSA Generic Watchdog",
         .options        = WDIOF_SETTIMEOUT |

@@ -186,6 +207,9 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
         struct resource *res;
         u32 status;
         int ret;
+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+       int irq;
+#endif

         gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
         if (!gwdt)

@@ -202,6 +226,14 @@ static int sbsa_gwdt_probe(struct platform_device *pdev)
         if (IS_ERR(rf_base))
                 return PTR_ERR(rf_base);

+#ifdef CONFIG_ARM_SBSA_WATCHDOG_PANIC
+       irq = platform_get_irq(pdev, 0);
+       if (irq < 0) {
+               dev_err(dev, "unable to get ws0 interrupt.\n");
+               return irq;
+       }
+#endif
+

Can't the driver revert to single stage mode if platform_get_irq()
fails?  That way the value of 'irq' can be tested throughout the
_probe() function and the #ifdefs removed.