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
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
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"; + }; };
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: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Signed-off-by: Fu Wei fu.wei@linaro.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";
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");
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?
Hi Timur,
On 4 February 2016 at 01:48, Timur Tabi timur@codeaurora.org wrote:
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?
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.
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?
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.
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 ARM64depends on ARM_ARCH_TIMERselect WATCHDOG_COREhelpARM 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 modulewill 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.
+}
+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
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 ARM64depends on ARM_ARCH_TIMERselect WATCHDOG_COREhelpARM 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 modulewill 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
On Thu, Feb 04, 2016 at 01:18:42AM +0800, fu.wei@linaro.org wrote:
+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- /*
- Writing WRR for an explicit watchdog refresh.
- You can write anyting(like 0xc0ffee).
- */
- writel(0xc0ffee, gwdt->refresh_base + SBSA_GWDT_WRR);
- return 0;
+}
You might get in trouble for that. 0xd09f00d is probably less poisonous.
http://www.petpoisonhelpline.com/poison/caffeine/
Will
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.
Any reason why we can't just keep it simple and write 0?
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.
Any reason why we can't just keep it simple and write 0?
+1
Guenter
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.
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
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; }
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.
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
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.
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.
Fu Wei wrote:
Actually I am taking your suggestion to avoid touching WCV in interrupt routine. So even we have pre-timeout support , it is useless for this panic-on-half-timeout feature, because pre-timeout == timeout / 2 (always).
So maybe I misunderstand your suggestion, could you let me know : why we want pre-timeout here?
Maybe I'm confused.
For pre-timeout, I think the SBSA watchdog driver should support only half-timeout. That is, the user cannot configure the length of the pre-timeout with this driver. He can only enable it, and it is automatically set to 1/2 timeout.
So when pre-timeout occurs, the interrupt handler calls panic() or whatever it's supposed to do.
So "pre-timeout == timeout / 2 (always)" is exactly what we want.
On 4 February 2016 at 02:08, Timur Tabi timur@codeaurora.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.
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.
On 4 February 2016 at 02:26, Timur Tabi timur@codeaurora.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.
Fu Wei wrote:
As you know I have made the pre-timeout support patch, If people like it, i am happy to go on upstream it separately.
If we want to use pre-timeout here, user only can use get_pretimeout and disable panic by setting pretimeout to 0 but user can not really set pretimeout, because "pre-timeout == timeout / 2 (always)". if user want to change pretimeout, he/she has to set_time instead.
Ok, I think patches 4 and 5 should be combined, and I think the Kconfig entry should be removed and just use panic_enabled.
On 4 February 2016 at 02:45, Timur Tabi 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 :-)
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
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.
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
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
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
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 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.
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
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
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
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
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
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?
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.
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 moduleparameter
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
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 bymodule 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
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_WATCHDOGhelpARM 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 contextinfo 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
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_WATCHDOGhelpARM 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 contextinfo 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
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.
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
-
Fu Wei -
fu.wei＠linaro.org
-
Guenter Roeck -
Mathieu Poirier -
Thomas Petazzoni -
Timur Tabi -
Will Deacon