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

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

[fu.wei]

From: Fu Wei <fu.wei@linaro.org>

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

[PATCH v10 1/5] Documentation: add sbsa-gwdt driver documentation

[fu.wei]

From: Fu Wei <fu.wei@linaro.org>

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 <fu.wei@linaro.org>
---
 .../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@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

[PATCH v10 2/5] ARM64: add SBSA Generic Watchdog device node in foundation-v8.dts

[fu.wei]

From: Fu Wei <fu.wei@linaro.org>

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 <fu.wei@linaro.org>
---
 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@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

[PATCH v10 3/5] ARM64: add SBSA Generic Watchdog device node in amd-seattle-soc.dtsi

[fu.wei-QSEj5FYQhm4dnm+yROfE0A]

From: Fu Wei <fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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-r2nGTMty4D4@public.gmane.org>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
Signed-off-by: Fu Wei <fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 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@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@e1020000 {
 			status = "disabled";
 			compatible = "arm,pl022", "arm,primecell";
-- 
2.5.0

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

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

[fu.wei]

From: Fu Wei <fu.wei@linaro.org>

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 <fu.wei@linaro.org>
Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>
Tested-by: Pratyush Anand <panand@redhat.com>
---
 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

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

[Timur Tabi]

fu.wei@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?

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

[Fu Wei]

Hi Timur,

On 4 February 2016 at 01:48, Timur Tabi <timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Timur Tabi]

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Fu Wei]

On 4 February 2016 at 01:58, Timur Tabi <timur@codeaurora.org> wrote:
> 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

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

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

[Timur Tabi]

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.

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

[Mathieu Poirier]

On 3 February 2016 at 10:18,  <fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> From: Fu Wei <fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> 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 <fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Reviewed-by: Graeme Gregory <graeme.gregory-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Tested-by: Pratyush Anand <panand-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *         Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> + *         Al Stone <al.stone-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> + *         Timur Tabi <timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.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;

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 <fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>");
> +MODULE_AUTHOR("Suravee Suthikulpanit <Suravee.Suthikulpanit-5C7GfCeVMHo@public.gmane.org>");
> +MODULE_AUTHOR("Al Stone <al.stone-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>");
> +MODULE_AUTHOR("Timur Tabi <timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.5.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Fu Wei]

On 5 February 2016 at 00:25, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
> On 3 February 2016 at 10:18,  <fu.wei@linaro.org> wrote:
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> 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 <fu.wei@linaro.org>
>> Reviewed-by: Graeme Gregory <graeme.gregory@linaro.org>
>> Tested-by: Pratyush Anand <panand@redhat.com>
>> ---
>>  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;
>
> 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.

OK, will do , thanks

>
>> +}
>> +
>> +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
>>



-- 
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

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

[Will Deacon]

On Thu, Feb 04, 2016 at 01:18:42AM +0800, fu.wei-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Timur Tabi]

Will Deacon 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/

Any reason why we can't just keep it simple and write 0?
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Guenter Roeck]

On 02/04/2016 08:37 AM, Timur Tabi wrote:
> Will Deacon 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/
>
> Any reason why we can't just keep it simple and write 0?

+1

Guenter

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

[Fu Wei]

Hi

On 5 February 2016 at 00:46, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/04/2016 08:37 AM, Timur Tabi wrote:
>>
>> Will Deacon 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/
>>
>>
>> Any reason why we can't just keep it simple and write 0?
>
>
> +1

yes, we can, just "0" would be fine, will do.

Thanks :-)

>
> Guenter
>



-- 
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

[PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[fu.wei]

From: Fu Wei <fu.wei@linaro.org>

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 <fu.wei@linaro.org>
---
 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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

fu.wei@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.

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

Hi Timur,

Thanks for your rapid feedback :-)

On 4 February 2016 at 01:27, Timur Tabi <timur@codeaurora.org> wrote:
> fu.wei@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

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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

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?

>> >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.

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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

Hi Timur

On 4 February 2016 at 01:53, Timur Tabi <timur@codeaurora.org> wrote:
> 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?

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?


>
>>> >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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

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.

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

On 4 February 2016 at 02:08, Timur Tabi <timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

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
>
>> >
>> >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.

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

On 4 February 2016 at 02:26, Timur Tabi <timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> 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
>>
>>> >
>>> >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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

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.

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

On 4 February 2016 at 02:45, Timur Tabi <timur@codeaurora.org> wrote:
> 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.

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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Guenter Roeck]

On 02/03/2016 03:00 PM, Fu Wei wrote:
> On 4 February 2016 at 02:45, Timur Tabi <timur@codeaurora.org> wrote:
>> 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.

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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Guenter Roeck]

On 02/04/2016 05:48 AM, Timur Tabi wrote:
> 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.
>

"* 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

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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

Hi Guenter,

On 4 February 2016 at 13:17, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>
>> On 4 February 2016 at 02:45, Timur Tabi <timur-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>>
>>> 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.
>
>
> Agreed.

np, will do

>>
>>
>> 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.

OK, my thought is

if panic is enabled :
|--------WOR-------WS0--------WOR-------WS1
|------timeout------(panic)------timeout-----reset

if panic is disabled .
|--------WOR-------WS0--------WOR-------WS1
|---------------------timeout---------------------reset

 panic_enabled only can be configured when module is loaded by module parameter

But user should know that max_timeout(panic_enable) =
max_timeout(panic_disable) / 2

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

yes, makes sense. Maybe we can do something  like this:

/*
 * action refers to action taken when watchdog gets WS0
 * 0 = SKIP
 * 1 = PANIC
 * defaults to SKIP (0)
 */
static int action;
module_param(action, int, 0);
MODULE_PARM_DESC(action, "after watchdog gets WS0 interrupt, do: "
"0 = SKIP(*)  1 = PANIC");


>
> Thanks,
> Guenter
>



-- 
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Thomas Petazzoni]

Hello,

On Fri, 5 Feb 2016 17:51:52 +0800, Fu Wei wrote:

> OK, my thought is
> 
> if panic is enabled :
> |--------WOR-------WS0--------WOR-------WS1
> |------timeout------(panic)------timeout-----reset

I'm quite certainly missing something completely obvious here, but how
can you get the WS1 interrupt *after* raising a panic? Aren't all
interrupts disabled and the system fully halted once you get a panic(),
especially when raised from an interrupt handler? If that's the case,
how can the system continue to do things, such as receiving the WS1
interrupt and resetting ?

Again, I'm probably missing something obvious, but I'm interested to
understand the reasoning here.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

Thomas Petazzoni wrote:
>> if panic is enabled :
>> >|--------WOR-------WS0--------WOR-------WS1
>> >|------timeout------(panic)------timeout-----reset

> I'm quite certainly missing something completely obvious here, but how
> can you get the WS1 interrupt*after*  raising a panic? Aren't all
> interrupts disabled and the system fully halted once you get a panic(),
> especially when raised from an interrupt handler? If that's the case,
> how can the system continue to do things, such as receiving the WS1
> interrupt and resetting ?

Typically, WS1 is not an interrupt.  Instead, it's a hard system-level 
reset.

The hardware is capable of generating an interrupt for both WS0 and WS1. 
  However, the ACPI table only contains one interrupt value, and it's 
not clear whether that's supposed to be the WS0 interrupt or the WS1 
interrupts.

So this whole thing does assume a specfic watchdog configuration.

-- 
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.

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Thomas Petazzoni]

Hello,

On Fri, 5 Feb 2016 07:08:23 -0600, Timur Tabi wrote:

> > I'm quite certainly missing something completely obvious here, but how
> > can you get the WS1 interrupt*after*  raising a panic? Aren't all
> > interrupts disabled and the system fully halted once you get a panic(),
> > especially when raised from an interrupt handler? If that's the case,
> > how can the system continue to do things, such as receiving the WS1
> > interrupt and resetting ?
> 
> Typically, WS1 is not an interrupt.  Instead, it's a hard system-level 
> reset.

Ah, right, true. I missed that aspect because on my HW, triggering a
system-level reset on WS1 is optional. I can actually get an interrupt
on both WS0 and WS1, and no reset at all.

But a normal configuration indeed involves having the WS1 event
configured in HW to be a system-level reset.

So, OK, it makes sense. Thanks for the clarification!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Guenter Roeck]

On 02/05/2016 01:51 AM, Fu Wei wrote:
> Hi Guenter,
>
> On 4 February 2016 at 13:17, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>
>>> On 4 February 2016 at 02:45, Timur Tabi <timur@codeaurora.org> wrote:
>>>>
>>>> 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.
>>
>>
>> Agreed.
>
> np, will do
>
>>>
>>>
>>> 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.
>
> OK, my thought is
>
> if panic is enabled :
> |--------WOR-------WS0--------WOR-------WS1
> |------timeout------(panic)------timeout-----reset
>
> if panic is disabled .
> |--------WOR-------WS0--------WOR-------WS1
> |---------------------timeout---------------------reset
>
>   panic_enabled only can be configured when module is loaded by module parameter
>
> But user should know that max_timeout(panic_enable) =
> max_timeout(panic_disable) / 2
>

That means you'll have to update max_timeout accordingly.

>>
>> I am not too happy with the parameter name (panic_enabled). How about
>> "action", to match machzwd ?
>
> yes, makes sense. Maybe we can do something  like this:
>
> /*
>   * action refers to action taken when watchdog gets WS0
>   * 0 = SKIP
>   * 1 = PANIC
>   * defaults to SKIP (0)
>   */
> static int action;
> module_param(action, int, 0);
> MODULE_PARM_DESC(action, "after watchdog gets WS0 interrupt, do: "
> "0 = SKIP(*)  1 = PANIC");
>
Yes, though I would suggest to use lower case letters.

Thanks,
Guenter

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

On 5 February 2016 at 22:42, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/05/2016 01:51 AM, Fu Wei wrote:
>>
>> Hi Guenter,
>>
>> On 4 February 2016 at 13:17, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>>
>>>>
>>>> On 4 February 2016 at 02:45, Timur Tabi <timur@codeaurora.org> wrote:
>>>>>
>>>>>
>>>>> 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.
>>>
>>>
>>>
>>> Agreed.
>>
>>
>> np, will do
>>
>>>>
>>>>
>>>> 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.
>>
>>
>> OK, my thought is
>>
>> if panic is enabled :
>> |--------WOR-------WS0--------WOR-------WS1
>> |------timeout------(panic)------timeout-----reset
>>
>> if panic is disabled .
>> |--------WOR-------WS0--------WOR-------WS1
>> |---------------------timeout---------------------reset
>>
>>   panic_enabled only can be configured when module is loaded by module
>> parameter
>>
>> But user should know that max_timeout(panic_enable) =
>> max_timeout(panic_disable) / 2
>>
>
> That means you'll have to update max_timeout accordingly.

panic_enabled only can be configured when module is loaded, so we
don't need to update it.

max_timeout will only be set up in the init stage.

Does it make sense ? :-)

>
>>>
>>> I am not too happy with the parameter name (panic_enabled). How about
>>> "action", to match machzwd ?
>>
>>
>> yes, makes sense. Maybe we can do something  like this:
>>
>> /*
>>   * action refers to action taken when watchdog gets WS0
>>   * 0 = SKIP
>>   * 1 = PANIC
>>   * defaults to SKIP (0)
>>   */
>> static int action;
>> module_param(action, int, 0);
>> MODULE_PARM_DESC(action, "after watchdog gets WS0 interrupt, do: "
>> "0 = SKIP(*)  1 = PANIC");
>>
> Yes, though I would suggest to use lower case letters.

yes,  NP, will do , Thanks :-)

>
> Thanks,
> Guenter
>



-- 
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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Guenter Roeck]

On 02/05/2016 10:21 AM, Fu Wei wrote:
> On 5 February 2016 at 22:42, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 02/05/2016 01:51 AM, Fu Wei wrote:
>>>
>>> Hi Guenter,
>>>
>>> On 4 February 2016 at 13:17, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>>>
>>>>>
>>>>> On 4 February 2016 at 02:45, Timur Tabi <timur@codeaurora.org> wrote:
>>>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>>
>>>>
>>>> Agreed.
>>>
>>>
>>> np, will do
>>>
>>>>>
>>>>>
>>>>> 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.
>>>
>>>
>>> OK, my thought is
>>>
>>> if panic is enabled :
>>> |--------WOR-------WS0--------WOR-------WS1
>>> |------timeout------(panic)------timeout-----reset
>>>
>>> if panic is disabled .
>>> |--------WOR-------WS0--------WOR-------WS1
>>> |---------------------timeout---------------------reset
>>>
>>>    panic_enabled only can be configured when module is loaded by module
>>> parameter
>>>
>>> But user should know that max_timeout(panic_enable) =
>>> max_timeout(panic_disable) / 2
>>>
>>
>> That means you'll have to update max_timeout accordingly.
>
> panic_enabled only can be configured when module is loaded, so we
> don't need to update it.
>
> max_timeout will only be set up in the init stage.
>
> Does it make sense ? :-)
>
Not sure I understand your problem or question.

max_timeout will have to reflect the correct maximum timeout, under
all circumstances. It will have to be set to the correct value before
the watchdog driver is registered.

Guenter

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

Hi Guenter,

On 6 February 2016 at 07:54, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/05/2016 10:21 AM, Fu Wei wrote:
>>
>> On 5 February 2016 at 22:42, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 02/05/2016 01:51 AM, Fu Wei wrote:
>>>>
>>>>
>>>> Hi Guenter,
>>>>
>>>> On 4 February 2016 at 13:17, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>
>>>>>
>>>>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 4 February 2016 at 02:45, Timur Tabi <timur@codeaurora.org> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Agreed.
>>>>
>>>>
>>>>
>>>> np, will do
>>>>
>>>>>>
>>>>>>
>>>>>> 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.
>>>>
>>>>
>>>>
>>>> OK, my thought is
>>>>
>>>> if panic is enabled :
>>>> |--------WOR-------WS0--------WOR-------WS1
>>>> |------timeout------(panic)------timeout-----reset
>>>>
>>>> if panic is disabled .
>>>> |--------WOR-------WS0--------WOR-------WS1
>>>> |---------------------timeout---------------------reset
>>>>
>>>>    panic_enabled only can be configured when module is loaded by module
>>>> parameter
>>>>
>>>> But user should know that max_timeout(panic_enable) =
>>>> max_timeout(panic_disable) / 2
>>>>
>>>
>>> That means you'll have to update max_timeout accordingly.
>>
>>
>> panic_enabled only can be configured when module is loaded, so we
>> don't need to update it.
>>
>> max_timeout will only be set up in the init stage.
>>
>> Does it make sense ? :-)
>>
> Not sure I understand your problem or question.
>
> max_timeout will have to reflect the correct maximum timeout, under
> all circumstances. It will have to be set to the correct value before
> the watchdog driver is registered.

yes, understood,  my thought is :

in  static int sbsa_gwdt_probe(struct platform_device *pdev)

if (action) {
        wdd->min_timeout = 1;
        wdd->max_timeout = U32_MAX / gwdt->clk;
} else {
        wdd->min_timeout = 2;
        wdd->max_timeout = U32_MAX / gwdt->clk * 2;
}



>
> Guenter
>



-- 
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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

Fu Wei wrote:
> if (action) {
>          wdd->min_timeout = 1;
>          wdd->max_timeout = U32_MAX / gwdt->clk;
> } else {
>          wdd->min_timeout = 2;
>          wdd->max_timeout = U32_MAX / gwdt->clk * 2;
> }

Why would the minimum timeout be 2?  You can program WOR to timeout in 
half a second, can't you?

-- 
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.

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

On 7 February 2016 at 02:55, Timur Tabi <timur@codeaurora.org> wrote:
> Fu Wei wrote:
>>
>> if (action) {
>>          wdd->min_timeout = 1;
>>          wdd->max_timeout = U32_MAX / gwdt->clk;
>> } else {
>>          wdd->min_timeout = 2;
>>          wdd->max_timeout = U32_MAX / gwdt->clk * 2;
>> }
>
>
> Why would the minimum timeout be 2?  You can program WOR to timeout in half
> a second, can't you?

Yes, I think we can.
you are right, will fix that.

>
>
> --
> 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.



-- 
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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Guenter Roeck]

On 02/06/2016 10:02 AM, Fu Wei wrote:
> Hi Guenter,
>
> On 6 February 2016 at 07:54, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 02/05/2016 10:21 AM, Fu Wei wrote:
>>>
>>> On 5 February 2016 at 22:42, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>> On 02/05/2016 01:51 AM, Fu Wei wrote:
>>>>>
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> On 4 February 2016 at 13:17, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>
>>>>>>
>>>>>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 4 February 2016 at 02:45, Timur Tabi <timur@codeaurora.org> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Agreed.
>>>>>
>>>>>
>>>>>
>>>>> np, will do
>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>
>>>>>
>>>>> OK, my thought is
>>>>>
>>>>> if panic is enabled :
>>>>> |--------WOR-------WS0--------WOR-------WS1
>>>>> |------timeout------(panic)------timeout-----reset
>>>>>
>>>>> if panic is disabled .
>>>>> |--------WOR-------WS0--------WOR-------WS1
>>>>> |---------------------timeout---------------------reset
>>>>>
>>>>>     panic_enabled only can be configured when module is loaded by module
>>>>> parameter
>>>>>
>>>>> But user should know that max_timeout(panic_enable) =
>>>>> max_timeout(panic_disable) / 2
>>>>>
>>>>
>>>> That means you'll have to update max_timeout accordingly.
>>>
>>>
>>> panic_enabled only can be configured when module is loaded, so we
>>> don't need to update it.
>>>
>>> max_timeout will only be set up in the init stage.
>>>
>>> Does it make sense ? :-)
>>>
>> Not sure I understand your problem or question.
>>
>> max_timeout will have to reflect the correct maximum timeout, under
>> all circumstances. It will have to be set to the correct value before
>> the watchdog driver is registered.
>
> yes, understood,  my thought is :
>
> in  static int sbsa_gwdt_probe(struct platform_device *pdev)
>
> if (action) {
>          wdd->min_timeout = 1;
>          wdd->max_timeout = U32_MAX / gwdt->clk;
> } else {
>          wdd->min_timeout = 2;
>          wdd->max_timeout = U32_MAX / gwdt->clk * 2;

Pretty much, though you would also have to adjust all calculations
using gwdt->clk, in both set_timeout() and get_timeout(). Wonder if
you could adjust gwdt->clk instead.

Does min_timeout really have to be 2 if panic is disabled ?
The only reason seems to be the calculation in sbsa_gwdt_set_timeout().

	writel(timeout / 2 * gwdt->clk, gwdt->control_base + SBSA_GWDT_WOR);

Maybe you could use something like

	writel(timeout * (gwdt->clk / 2), ...);

instead. Or, as mentioned above, adjust the value of gwdt->clk
to include the factor.

Thanks,
Guenter

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

On 7 February 2016 at 02:57, Guenter Roeck <linux@roeck-us.net> wrote:
> On 02/06/2016 10:02 AM, Fu Wei wrote:
>>
>> Hi Guenter,
>>
>> On 6 February 2016 at 07:54, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 02/05/2016 10:21 AM, Fu Wei wrote:
>>>>
>>>>
>>>> On 5 February 2016 at 22:42, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>
>>>>>
>>>>> On 02/05/2016 01:51 AM, Fu Wei wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Guenter,
>>>>>>
>>>>>> On 4 February 2016 at 13:17, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 02/03/2016 03:00 PM, Fu Wei wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4 February 2016 at 02:45, Timur Tabi <timur@codeaurora.org>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Agreed.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> np, will do
>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> OK, my thought is
>>>>>>
>>>>>> if panic is enabled :
>>>>>> |--------WOR-------WS0--------WOR-------WS1
>>>>>> |------timeout------(panic)------timeout-----reset
>>>>>>
>>>>>> if panic is disabled .
>>>>>> |--------WOR-------WS0--------WOR-------WS1
>>>>>> |---------------------timeout---------------------reset
>>>>>>
>>>>>>     panic_enabled only can be configured when module is loaded by
>>>>>> module
>>>>>> parameter
>>>>>>
>>>>>> But user should know that max_timeout(panic_enable) =
>>>>>> max_timeout(panic_disable) / 2
>>>>>>
>>>>>
>>>>> That means you'll have to update max_timeout accordingly.
>>>>
>>>>
>>>>
>>>> panic_enabled only can be configured when module is loaded, so we
>>>> don't need to update it.
>>>>
>>>> max_timeout will only be set up in the init stage.
>>>>
>>>> Does it make sense ? :-)
>>>>
>>> Not sure I understand your problem or question.
>>>
>>> max_timeout will have to reflect the correct maximum timeout, under
>>> all circumstances. It will have to be set to the correct value before
>>> the watchdog driver is registered.
>>
>>
>> yes, understood,  my thought is :
>>
>> in  static int sbsa_gwdt_probe(struct platform_device *pdev)
>>
>> if (action) {
>>          wdd->min_timeout = 1;
>>          wdd->max_timeout = U32_MAX / gwdt->clk;
>> } else {
>>          wdd->min_timeout = 2;
>>          wdd->max_timeout = U32_MAX / gwdt->clk * 2;
>
>
> Pretty much, though you would also have to adjust all calculations
> using gwdt->clk, in both set_timeout() and get_timeout(). Wonder if
> you could adjust gwdt->clk instead.
>
> Does min_timeout really have to be 2 if panic is disabled ?
> The only reason seems to be the calculation in sbsa_gwdt_set_timeout().
>
>         writel(timeout / 2 * gwdt->clk, gwdt->control_base + SBSA_GWDT_WOR);
>
> Maybe you could use something like
>
>         writel(timeout * (gwdt->clk / 2), ...);
>
> instead. Or, as mentioned above, adjust the value of gwdt->clk
> to include the factor.

yes,  Thanks for pointing it out.
I will fix that following your suggestion.

>
> Thanks,
> Guenter
>



-- 
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

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Mathieu Poirier]

On 3 February 2016 at 10:18,  <fu.wei@linaro.org> wrote:
> From: Fu Wei <fu.wei@linaro.org>
>
> 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 <fu.wei@linaro.org>
> ---
>  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.

Thanks,
Mathieu

>         /*
>          * 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
>

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Guenter Roeck]

On 02/04/2016 08:32 AM, Mathieu Poirier wrote:
> On 3 February 2016 at 10:18,  <fu.wei@linaro.org> wrote:
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> 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 <fu.wei@linaro.org>
>> ---
>>   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.
>
Good point. Making irq support mandatory on all systems isn't really
a good idea.

Thanks,
Guenter

> Thanks,
> Mathieu
>
>>          /*
>>           * 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
>>
>

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Timur Tabi]

Mathieu Poirier wrote:
>> >+#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.

I like that idea.  The same can be done with the devm_request_irq() 
call.  It should definitely still display a warning if the command-line 
option is set but no interrupt is available.

Re: [PATCH v10 5/5] Watchdog: ARM SBSA Generic Watchdog half timeout panic support

[Fu Wei]

On 5 February 2016 at 00:43, Timur Tabi <timur@codeaurora.org> wrote:
> Mathieu Poirier wrote:
>>>
>>> >+#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.
>
>
> I like that idea.  The same can be done with the devm_request_irq() call.
> It should definitely still display a warning if the command-line option is
> set but no interrupt is available.

Yes, I agree with that too, brilliant idea, this will be in v11 patchset



-- 
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