From: Fu Wei fu.wei@linaro.org
This patchset:
(1)Export "arch_timer_get_rate" in arm_arch_timer.c for the other drivers, like SBSA watchdog driver
(2)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.
(3)Introduce "pretimeout" into the watchdog framework, and update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
(4)Introduce ARM SBSA watchdog driver: a.Use linux kernel watchdog framework; b.Work with FDT on ARM64; c.Use "pretimeout" in watchdog framework; d.In first timeout(WS0), do panic to save system context; e.Support geting timeout and pretimeout from parameter and FDT at the driver init stage.
(5)Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by This Watchdog driver.
This patchset has been tested with watchdog daemon (ACPI/FDT, module/build-in) on the following platforms: (1)ARM Foundation v8 model (2)AMD Seattle B0
Changelog: v2: 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: The first version upstream patchset to linux mailing list
Fu Wei (7): clocksource: export "arch_timer_get_rate" for the other drivers Documentation: add sbsa-gwdt.txt 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 "pretimeout" into framework Watchdog: introduce ARM SBSA watchdog driver ACPI: import watchdog info of GTDT into platform device
.../devicetree/bindings/watchdog/sbsa-gwdt.txt | 36 ++ Documentation/watchdog/watchdog-kernel-api.txt | 62 ++- arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 11 + arch/arm64/boot/dts/arm/foundation-v8.dts | 10 + arch/arm64/kernel/acpi.c | 136 ++++++ drivers/clocksource/arm_arch_timer.c | 1 + drivers/watchdog/Kconfig | 12 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++ drivers/watchdog/watchdog_core.c | 103 +++-- drivers/watchdog/watchdog_dev.c | 48 +++ include/linux/watchdog.h | 30 +- 12 files changed, 891 insertions(+), 35 deletions(-) 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
Some devices get clock from system counter, like SBSA watchdog driver. They may need to get system counter rate.
Signed-off-by: Fu Wei fu.wei@linaro.org --- drivers/clocksource/arm_arch_timer.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 0aa135d..4327bf9 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -410,6 +410,7 @@ u32 arch_timer_get_rate(void) { return arch_timer_rate; } +EXPORT_SYMBOL_GPL(arch_timer_get_rate);
static u64 arch_counter_get_cntvct_mem(void) {
On 2015年05月21日 16:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Some devices get clock from system counter, like SBSA watchdog driver. They may need to get system counter rate.
and please add a comment that SBSA watchdog is a kernel module, then it would explicit that why EXPORT_SYMBOL_GPL is needed.
Signed-off-by: Fu Wei fu.wei@linaro.org
Other than that,
Acked-by: Hanjun Guo hanjun.guo@linaro.org
drivers/clocksource/arm_arch_timer.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index 0aa135d..4327bf9 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -410,6 +410,7 @@ u32 arch_timer_get_rate(void) { return arch_timer_rate; } +EXPORT_SYMBOL_GPL(arch_timer_get_rate);
static u64 arch_counter_get_cntvct_mem(void) {
fu.wei@linaro.org wrote:
Some devices get clock from system counter, like SBSA watchdog driver. They may need to get system counter rate.
We don't need this patch. The watchdog driver can use arch_timer_get_cntfrq() instead of arch_timer_get_rate(). There's nothing wrong with arch_timer_get_cntfrq() since the SBSA driver is intended only for ARM64 server systems, and arch_timer_get_cntfrq() is always defined for such systems.
On Fri, May 22, 2015 at 09:09:10AM -0500, Timur Tabi wrote:
fu.wei@linaro.org wrote:
Some devices get clock from system counter, like SBSA watchdog driver. They may need to get system counter rate.
We don't need this patch. The watchdog driver can use arch_timer_get_cntfrq() instead of arch_timer_get_rate(). There's nothing wrong with arch_timer_get_cntfrq() since the SBSA driver is intended only for ARM64 server systems, and arch_timer_get_cntfrq() is always defined for such systems.
I agree, since the SBSA driver also uses arch_counter_get_cntvct() which is declared in the same include file as arch_timer_get_cntfrq(). The additional dependency does not provide any value, unless architecture implementation dependencies can be removed entirely, which does not seem to be the case.
Doo bad that SBSA is not hardware independent enough to provide all the information needed to implement such a driver :-(.
Guenter
On 05/22/2015 10:16 AM, Guenter Roeck wrote:
Doo bad that SBSA is not hardware independent enough to provide all the information needed to implement such a driver:-(.
The SBSA is designed and intended only for ARM Servers. It has no value on non-ARM SOCs and and non-server SOCs. The SBSA is just a way for 64-bit ARM Server SOC vendors to re-use some drivers.
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.
Signed-off-by: Fu Wei fu.wei@linaro.org --- .../devicetree/bindings/watchdog/sbsa-gwdt.txt | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt new file mode 100644 index 0000000..bd1768d --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt @@ -0,0 +1,36 @@ +* SBSA(Server Base System Architecture) Generic Watchdog + +The SBSA Generic Watchdog Timer is used for resetting the system after +two stages of timeout. +More details: ARM-DEN-0029 - Server Base System Architecture (SBSA) + +Required properties: +- compatible : Should at least contain "arm,sbsa-gwdt". + +- reg : base physical address of the frames and length of memory mapped region. + +- reg-names : Should contain the resource reg names to show the order of + the values in "reg". + Must include the following entries : "refresh", "control". + +- interrupts : Should at least contain WS0 interrupt, + the WS1 Signal is optional. + +- interrupt-names : Should contain the resource interrupt names. + Must include the following entries : "ws0". "ws1" is optional. + +Optional properties +- timeout-sec : Watchdog pre-timeout and timeout values (in seconds). + The first is timeout values, then pre-timeout. + +Example for FVP Foundation Model v8: + +watchdog@2a450000 { + compatible = "arm,sbsa-gwdt"; + reg = <0x0 0x2a450000 0 0x10000>, + <0x0 0x2a440000 0 0x10000>; + reg-names = "refresh", "control"; + interrupts = <0 27 4>; + interrupt-names = "ws0"; + timeout-sec = <10 5>; +};
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.
Signed-off-by: Fu Wei fu.wei@linaro.org --- arch/arm64/boot/dts/arm/foundation-v8.dts | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/arm/foundation-v8.dts b/arch/arm64/boot/dts/arm/foundation-v8.dts index 4eac8dc..e3fae16 100644 --- a/arch/arm64/boot/dts/arm/foundation-v8.dts +++ b/arch/arm64/boot/dts/arm/foundation-v8.dts @@ -237,4 +237,14 @@ }; }; }; + watchdog0: watchdog@2a450000 { + compatible = "arm,sbsa-gwdt"; + reg = <0x0 0x2a450000 0 0x10000>, + <0x0 0x2a440000 0 0x10000>; + reg-names = "refresh", + "control"; + interrupts = <0 27 4>; + interrupt-names = "ws0"; + timeout-sec = <10 5>; + }; };
On Thursday 21 May 2015 16:32:32 fu.wei@linaro.org wrote:
watchdog0: watchdog@2a450000 {compatible = "arm,sbsa-gwdt";reg = <0x0 0x2a450000 0 0x10000>,<0x0 0x2a440000 0 0x10000>;reg-names = "refresh","control";interrupts = <0 27 4>;interrupt-names = "ws0";timeout-sec = <10 5>;};};
just one tiny comment: you can drop the 'watchdog0:' label here, there is normally no reason to assign a label to a watchdog device, especially if it's in a .dts file rather than a .dtsi.
The same is true for the example in the binding documentation.
Arnd
Hi Arnd,
yes, you are right , will delete it :-)
On 21 May 2015 at 16:45, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 21 May 2015 16:32:32 fu.wei@linaro.org wrote:
watchdog0: watchdog@2a450000 {compatible = "arm,sbsa-gwdt";reg = <0x0 0x2a450000 0 0x10000>,<0x0 0x2a440000 0 0x10000>;reg-names = "refresh","control";interrupts = <0 27 4>;interrupt-names = "ws0";timeout-sec = <10 5>;};};
just one tiny comment: you can drop the 'watchdog0:' label here, there is normally no reason to assign a label to a watchdog device, especially if it's in a .dts file rather than a .dtsi.
The same is true for the example in the binding documentation.
Arnd
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.
Signed-off-by: Fu Wei fu.wei@linaro.org --- arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 11 +++++++++++ 1 file changed, 11 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..95994eb 100644 --- a/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi +++ b/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi @@ -84,6 +84,17 @@ clock-names = "uartclk", "apb_pclk"; };
+ watchdog0: watchdog@e0bb0000 { + compatible = "arm,sbsa-gwdt"; + reg = <0x0 0xe0bb0000 0 0x10000>, + <0x0 0xe0bc0000 0 0x10000>; + reg-names = "refresh", + "control"; + interrupts = <0 337 4>; + interrupt-names = "ws0"; + timeout-sec = <10 5>; + }; + spi0: ssp@e1020000 { status = "disabled"; compatible = "arm,pl022", "arm,primecell";
On 5/21/15 03:32, fu.wei@linaro.org wrote:
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.
Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 11 +++++++++++ 1 file changed, 11 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..95994eb 100644 --- a/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi +++ b/arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi @@ -84,6 +84,17 @@ clock-names = "uartclk", "apb_pclk"; };
watchdog0: watchdog@e0bb0000 {compatible = "arm,sbsa-gwdt";reg = <0x0 0xe0bb0000 0 0x10000>,<0x0 0xe0bc0000 0 0x10000>;reg-names = "refresh","control";interrupts = <0 337 4>;interrupt-names = "ws0";timeout-sec = <10 5>;};- spi0: ssp@e1020000 { status = "disabled"; compatible = "arm,pl022", "arm,primecell";
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Acked-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org --- Documentation/watchdog/watchdog-kernel-api.txt | 62 ++++++++++++--- drivers/watchdog/watchdog_core.c | 103 +++++++++++++++++++------ drivers/watchdog/watchdog_dev.c | 48 ++++++++++++ include/linux/watchdog.h | 30 ++++++- 4 files changed, 208 insertions(+), 35 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index a0438f3..43900df 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -49,6 +49,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout; + unsigned int pretimeout; + unsigned int min_pretimeout; + unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status; @@ -70,6 +73,9 @@ It contains following fields: * timeout: the watchdog timer's timeout value (in seconds). * min_timeout: the watchdog timer's minimum timeout value (in seconds). * max_timeout: the watchdog timer's maximum timeout value (in seconds). +* pretimeout: the watchdog timer's pretimeout value (in seconds). +* min_pretimeout: the watchdog timer's minimum pretimeout value (in seconds). +* max_pretimeout: the watchdog timer's maximum pretimeout value (in seconds). * bootstatus: status of the device after booting (reported with watchdog WDIOF_* status bits). * driver_data: a pointer to the drivers private data of a watchdog device. @@ -92,6 +98,7 @@ struct watchdog_ops { int (*ping)(struct watchdog_device *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); + int (*set_pretimeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *); @@ -153,9 +160,19 @@ they are supported. These optional routines/operations are: and -EIO for "could not write value to the watchdog". On success this routine should set the timeout value of the watchdog_device to the achieved timeout value (which may be different from the requested one - because the watchdog does not necessarily has a 1 second resolution). + because the watchdog does not necessarily has a 1 second resolution; + If the driver supports pretimeout, then the timeout value must be greater + than that). (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the watchdog's info structure). +* set_pretimeout: this routine checks and changes the pretimeout of the + watchdog timer device. It returns 0 on success, -EINVAL for "parameter out of + range" and -EIO for "could not write value to the watchdog". On success this + routine should set the pretimeout value of the watchdog_device to the + achieved pretimeout value (which may be different from the requested one + because the watchdog does not necessarily has a 1 second resolution). + (Note: the WDIOF_PRETIMEOUT needs to be set in the options field of the + watchdog's info structure). * get_timeleft: this routines returns the time that's left before a reset. * ref: the operation that calls kref_get on the kref of a dynamically allocated watchdog_device struct. @@ -213,14 +230,41 @@ The watchdog_get_drvdata function allows you to retrieve driver specific data. The argument of this function is the watchdog device where you want to retrieve data from. The function returns the pointer to the driver specific data.
-To initialize the timeout field, the following function can be used: +To initialize the timeout and pretimeout fields, the following function can be +used:
-extern int watchdog_init_timeout(struct watchdog_device *wdd, - unsigned int timeout_parm, struct device *dev); +extern int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + void (*update_limits)(struct watchdog_device *), + struct device *dev);
-The watchdog_init_timeout function allows you to initialize the timeout field -using the module timeout parameter or by retrieving the timeout-sec property from -the device tree (if the module timeout parameter is invalid). Best practice is -to set the default timeout value as timeout value in the watchdog_device and -then use this function to set the user "preferred" timeout value. +The watchdog_init_timeouts function allows you to initialize the pretimeout and +timeout fields using the module pretimeout and timeout parameter or by +retrieving the elements in the timeout-sec property(the first element is for +timeout, the second one is for pretimeout) from the device tree(if the module +pretimeout and timeout parameter are invalid). +Normally, the pretimeout value will affect the limitation of timeout, and it +is also hardware related. So you can write a function in your driver to update +the limitation of timeout, according to the pretimeout value. Then pass the +function pointer by the update_limits parameter. If you driver doesn't +need this adjustment, just pass NULL to the update_limits parameter. +Most of watchdog timers have not pretimeout as the warning signal. They just +reset the system, once the timeout watch period expires. In this case, we can +pass 0 to the pretimeout_parm, and pass NULL to the update_limits. Or use the +old API: watchdog_init_timeout(see below) +Best practice is to set the default pretimeout and timeout value as pretimeout +and timeout value in the watchdog_device and then use this function to set the +user "preferred" pretimeout value. This routine returns zero on success and a negative errno code for failure. + +For backward compatibility, we also support the timeout initialization API: + +static inline int watchdog_init_timeout(struct watchdog_device *wdd, + unsigned int timeout_parm, + struct device *dev); + +Because of the introduction of pretimeout and watchdog_init_timeouts, this +function has become a small wrapper function of watchdog_init_timeouts. + + diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index cec9b55..460796e 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -43,60 +43,115 @@ static DEFINE_IDA(watchdog_ida); static struct class *watchdog_class;
-static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) +static void watchdog_check_min_max_timeouts(struct watchdog_device *wdd) { /* - * Check that we have valid min and max timeout values, if - * not reset them both to 0 (=not used or unknown) + * Check that we have valid min and max pretimeout and timeout values, + * if not reset them both to 0 (=not used or unknown) */ + if (wdd->min_pretimeout > wdd->max_pretimeout) { + pr_info("Invalid min and max pretimeout, resetting to 0!\n"); + wdd->min_pretimeout = 0; + wdd->max_pretimeout = 0; + } if (wdd->min_timeout > wdd->max_timeout) { pr_info("Invalid min and max timeout values, resetting to 0!\n"); wdd->min_timeout = 0; wdd->max_timeout = 0; } + /* + * Check that we have valid min and max timeout values, + * if not reset them both to pretimeout limits + */ + if (wdd->min_pretimeout && wdd->min_timeout < wdd->min_pretimeout) { + pr_info("Invalid min timeout, resetting to min pretimeout!\n"); + wdd->min_timeout = wdd->min_pretimeout; + } + if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout) { + pr_info("Invalid max timeout, resetting to max pretimeout!\n"); + wdd->max_timeout = wdd->max_pretimeout; + } }
/** - * watchdog_init_timeout() - initialize the timeout field + * watchdog_init_timeouts() - initialize the pretimeout and timeout field + * @pretimeout_parm: pretimeout module parameter * @timeout_parm: timeout module parameter * @dev: Device that stores the timeout-sec property * - * Initialize the timeout field of the watchdog_device struct with either the - * timeout module parameter (if it is valid value) or the timeout-sec property - * (only if it is a valid value and the timeout_parm is out of bounds). - * If none of them are valid then we keep the old value (which should normally - * be the default timeout value. + * Initialize the pretimeout and timeout field of the watchdog_device struct + * with either the pretimeout and timeout module parameter (if it is valid + * value) or the timeout-sec property (only if it is a valid value and the + * pretimeout_parm and timeout_parm is out of bounds). If none of them are + * valid then we keep the old value (which should normally be the default + * timeout value. * * A zero is returned on success and -EINVAL for failure. */ -int watchdog_init_timeout(struct watchdog_device *wdd, - unsigned int timeout_parm, struct device *dev) +int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + void (*update_limits)(struct watchdog_device *), + struct device *dev) { - unsigned int t = 0; + unsigned int timeout = 0, pretimeout = 0; + const __be32 *ppretimeout; int ret = 0; + struct property *timeout_sec; + int length;
- watchdog_check_min_max_timeout(wdd); + watchdog_check_min_max_timeouts(wdd);
- /* try to get the timeout module parameter first */ - if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) { - wdd->timeout = timeout_parm; - return ret; + /* try to get the timeout and pretimeout module parameter first */ + if (pretimeout_parm) { + if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm)) { + wdd->pretimeout = pretimeout_parm; + if (update_limits) + update_limits(wdd); + } else { + pr_warn("Invalid pretimeout parameter!\n"); + ret = -EINVAL; + } } - if (timeout_parm) + + if (timeout_parm) { + if (!watchdog_timeout_invalid(wdd, timeout_parm)) { + wdd->timeout = timeout_parm; + return ret; + } + pr_warn("Invalid timeout parameter!\n"); ret = -EINVAL; + }
/* try to get the timeout_sec property */ if (dev == NULL || dev->of_node == NULL) return ret; - of_property_read_u32(dev->of_node, "timeout-sec", &t); - if (!watchdog_timeout_invalid(wdd, t) && t) - wdd->timeout = t; - else + + timeout_sec = of_find_property(dev->of_node, "timeout-sec", &length); + if (timeout_sec) { + ppretimeout = of_prop_next_u32(timeout_sec, NULL, &timeout); + if (length == 2) { + of_prop_next_u32(timeout_sec, ppretimeout, &pretimeout); + if (!watchdog_pretimeout_invalid(wdd, pretimeout) && + pretimeout) { + wdd->pretimeout = pretimeout; + if (update_limits) + update_limits(wdd); + } else { + ret = -EINVAL; + } + } + if (!watchdog_timeout_invalid(wdd, timeout) && timeout) + wdd->timeout = timeout; + else + ret = -EINVAL; + } else { ret = -EINVAL; + }
return ret; } -EXPORT_SYMBOL_GPL(watchdog_init_timeout); +EXPORT_SYMBOL_GPL(watchdog_init_timeouts);
/** * watchdog_register_device() - register a watchdog device @@ -119,7 +174,7 @@ int watchdog_register_device(struct watchdog_device *wdd) if (wdd->ops->start == NULL || wdd->ops->stop == NULL) return -EINVAL;
- watchdog_check_min_max_timeout(wdd); + watchdog_check_min_max_timeouts(wdd);
/* * Note: now that all watchdog_device data has been verified, we diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 6aaefba..b519257 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -218,6 +218,38 @@ out_timeout: }
/* + * watchdog_set_pretimeout: set the watchdog timer pretimeout + * @wddev: the watchdog device to set the timeout for + * @pretimeout: pretimeout to set in seconds + */ + +static int watchdog_set_pretimeout(struct watchdog_device *wddev, + unsigned int pretimeout) +{ + int err; + + if (!wddev->ops->set_pretimeout || + !(wddev->info->options & WDIOF_PRETIMEOUT)) + return -EOPNOTSUPP; + + if (watchdog_pretimeout_invalid(wddev, pretimeout)) + return -EINVAL; + + mutex_lock(&wddev->lock); + + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_pretimeout; + } + + err = wddev->ops->set_pretimeout(wddev, pretimeout); + +out_pretimeout: + mutex_unlock(&wddev->lock); + return err; +} + +/* * watchdog_get_timeleft: wrapper to get the time left before a reboot * @wddev: the watchdog device to get the remaining time from * @timeleft: the time that's left @@ -388,6 +420,22 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd, if (wdd->timeout == 0) return -EOPNOTSUPP; return put_user(wdd->timeout, p); + case WDIOC_SETPRETIMEOUT: + if (get_user(val, p)) + return -EFAULT; + err = watchdog_set_pretimeout(wdd, val); + if (err < 0) + return err; + /* If the watchdog is active then we send a keepalive ping + * to make sure that the watchdog keep's running (and if + * possible that it takes the new timeout) */ + watchdog_ping(wdd); + /* Fall */ + case WDIOC_GETPRETIMEOUT: + /* timeout == 0 means that we don't use the pretimeout */ + if (wdd->pretimeout == 0) + return -EOPNOTSUPP; + return put_user(wdd->pretimeout, p); case WDIOC_GETTIMELEFT: err = watchdog_get_timeleft(wdd, &val); if (err) diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index a746bf5..df430a3 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -25,6 +25,7 @@ struct watchdog_device; * @ping: The routine that sends a keepalive ping to the watchdog device. * @status: The routine that shows the status of the watchdog device. * @set_timeout:The routine for setting the watchdog devices timeout value. + * @set_pretimeout:The routine for setting the watchdog devices pretimeout value * @get_timeleft:The routine that get's the time that's left before a reset. * @ref: The ref operation for dyn. allocated watchdog_device structs * @unref: The unref operation for dyn. allocated watchdog_device structs @@ -44,6 +45,7 @@ struct watchdog_ops { int (*ping)(struct watchdog_device *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); + int (*set_pretimeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *); @@ -62,6 +64,9 @@ struct watchdog_ops { * @timeout: The watchdog devices timeout value. * @min_timeout:The watchdog devices minimum timeout value. * @max_timeout:The watchdog devices maximum timeout value. + * @pretimeout: The watchdog devices pretimeout value. + * @min_pretimeout:The watchdog devices minimum pretimeout value. + * @max_pretimeout:The watchdog devices maximum pretimeout value. * @driver-data:Pointer to the drivers private data. * @lock: Lock for watchdog core internal use only. * @status: Field that contains the devices internal status bits. @@ -86,6 +91,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout; + unsigned int pretimeout; + unsigned int min_pretimeout; + unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status; @@ -120,6 +128,14 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne (t < wdd->min_timeout || t > wdd->max_timeout)); }
+/* Use the following function to check if a pretimeout value is invalid */ +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd, + unsigned int t) +{ + return ((wdd->max_pretimeout != 0) && + (t < wdd->min_pretimeout || t > wdd->max_pretimeout)); +} + /* Use the following functions to manipulate watchdog driver specific data */ static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data) { @@ -132,11 +148,21 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd) }
/* drivers/watchdog/watchdog_core.c */ -extern int watchdog_init_timeout(struct watchdog_device *wdd, - unsigned int timeout_parm, struct device *dev); +extern int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + void (*update_limits)(struct watchdog_device *), + struct device *dev); extern int watchdog_register_device(struct watchdog_device *); extern void watchdog_unregister_device(struct watchdog_device *);
+static inline int watchdog_init_timeout(struct watchdog_device *wdd, + unsigned int timeout_parm, + struct device *dev) +{ + return watchdog_init_timeouts(wdd, 0, timeout_parm, NULL, dev); +} + #ifdef CONFIG_HARDLOCKUP_DETECTOR void watchdog_nmi_disable_all(void); void watchdog_nmi_enable_all(void);
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org
[ ... ]
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
unsigned int pretimeout_parm,unsigned int timeout_parm,void (*update_limits)(struct watchdog_device *),struct device *dev);-The watchdog_init_timeout function allows you to initialize the timeout field -using the module timeout parameter or by retrieving the timeout-sec property from -the device tree (if the module timeout parameter is invalid). Best practice is -to set the default timeout value as timeout value in the watchdog_device and -then use this function to set the user "preferred" timeout value. +The watchdog_init_timeouts function allows you to initialize the pretimeout and +timeout fields using the module pretimeout and timeout parameter or by +retrieving the elements in the timeout-sec property(the first element is for +timeout, the second one is for pretimeout) from the device tree(if the module +pretimeout and timeout parameter are invalid). +Normally, the pretimeout value will affect the limitation of timeout, and it +is also hardware related. So you can write a function in your driver to update +the limitation of timeout, according to the pretimeout value. Then pass the +function pointer by the update_limits parameter. If you driver doesn't +need this adjustment, just pass NULL to the update_limits parameter.
You've lost me a bit with the update_limits function. watchdog_init_timeouts() is called from the driver. Why should the function have to call back into the driver to update the parameters which are passed from the driver ? Seems to me the driver can do that calculation first, then call watchdog_init_timeouts() with the result. Am I missing something ?
Thanks, Guenter
Hi Guenter,
Thanks for review. :-) feedback inline below
On 21 May 2015 at 17:04, Guenter Roeck linux@roeck-us.net wrote:
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org
[ ... ]
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
unsigned int pretimeout_parm,unsigned int timeout_parm,void (*update_limits)(structwatchdog_device *),
struct device *dev);-The watchdog_init_timeout function allows you to initialize the timeout field -using the module timeout parameter or by retrieving the timeout-sec property from -the device tree (if the module timeout parameter is invalid). Best practice is -to set the default timeout value as timeout value in the watchdog_device and -then use this function to set the user "preferred" timeout value. +The watchdog_init_timeouts function allows you to initialize the pretimeout and +timeout fields using the module pretimeout and timeout parameter or by +retrieving the elements in the timeout-sec property(the first element is for +timeout, the second one is for pretimeout) from the device tree(if the module +pretimeout and timeout parameter are invalid). +Normally, the pretimeout value will affect the limitation of timeout, and it +is also hardware related. So you can write a function in your driver to update +the limitation of timeout, according to the pretimeout value. Then pass the +function pointer by the update_limits parameter. If you driver doesn't +need this adjustment, just pass NULL to the update_limits parameter.
You've lost me a bit with the update_limits function. watchdog_init_timeouts() is called from the driver.
yes, that is the help function which will be called from watchdog driver, like SBSA watchdog driver
Why should the function have to call back into the driver to update the parameters which are passed from the driver ?
Let me explain this, please correct me if I misunderstand something. According to the concept of "pretimeout" in kernel, the timeout contains the pretimeout, like
* Kernel/API: P---------| pretimeout * |-------------------------------T timeout
If you set up the value of pretimeout, that means pretimeout <min_timeout < timeout < max_timeout < (pretimeout + max_timeout_for_1th_stage) For min_timeout > pretimeout. if some one setup a timeout like : pretimeout > timeout > min_timeout, I think that could be a problem For max_timeout < (pretimeout + max_timeout_for_1th_stage), if some one setup a timeout like (pretimeout + max_timeout_for_1th_stage) < timeout > max_timeout .
I have explained a little in doc, but the adjustment may have something to do with hardware, like max_timeout_for_1th_stage(in SBSA watchdog , limited by WCV)
maybe this problem wouldn't happen ,if you set up max_timeout to a small number. so you can pass NULL to the pointer. but I think maybe for other device , that may happen.
Seems to me the driver can do that calculation first, then call watchdog_init_timeouts() with the result. Am I missing something ?
maybe I am overthinking it :-) please correct me
Thanks, Guenter
On 05/21/2015 03:05 AM, Fu Wei wrote:
Hi Guenter,
Thanks for review. :-) feedback inline below
On 21 May 2015 at 17:04, Guenter Roeck linux@roeck-us.net wrote:
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org
[ ... ]
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
unsigned int pretimeout_parm,unsigned int timeout_parm,void (*update_limits)(structwatchdog_device *),
struct device *dev);-The watchdog_init_timeout function allows you to initialize the timeout field -using the module timeout parameter or by retrieving the timeout-sec property from -the device tree (if the module timeout parameter is invalid). Best practice is -to set the default timeout value as timeout value in the watchdog_device and -then use this function to set the user "preferred" timeout value. +The watchdog_init_timeouts function allows you to initialize the pretimeout and +timeout fields using the module pretimeout and timeout parameter or by +retrieving the elements in the timeout-sec property(the first element is for +timeout, the second one is for pretimeout) from the device tree(if the module +pretimeout and timeout parameter are invalid). +Normally, the pretimeout value will affect the limitation of timeout, and it +is also hardware related. So you can write a function in your driver to update +the limitation of timeout, according to the pretimeout value. Then pass the +function pointer by the update_limits parameter. If you driver doesn't +need this adjustment, just pass NULL to the update_limits parameter.
You've lost me a bit with the update_limits function. watchdog_init_timeouts() is called from the driver.
yes, that is the help function which will be called from watchdog driver, like SBSA watchdog driver
Why should the function have to call back into the driver to update the parameters which are passed from the driver ?
Let me explain this, please correct me if I misunderstand something. According to the concept of "pretimeout" in kernel, the timeout contains the pretimeout, like
- Kernel/API: P---------| pretimeout
|-------------------------------T timeoutIf you set up the value of pretimeout, that means pretimeout <min_timeout < timeout < max_timeout < (pretimeout + max_timeout_for_1th_stage) For min_timeout > pretimeout. if some one setup a timeout like : pretimeout > timeout > min_timeout, I think that could be a problem For max_timeout < (pretimeout + max_timeout_for_1th_stage), if some one setup a timeout like (pretimeout + max_timeout_for_1th_stage) < timeout > max_timeout .
I have explained a little in doc, but the adjustment may have something to do with hardware, like max_timeout_for_1th_stage(in SBSA watchdog , limited by WCV)
maybe this problem wouldn't happen ,if you set up max_timeout to a small number. so you can pass NULL to the pointer. but I think maybe for other device , that may happen.
Seems to me the driver can do that calculation first, then call watchdog_init_timeouts() with the result. Am I missing something ?
maybe I am overthinking it :-) please correct me
I just sent a more complete review. In general I think this problem (where the driver needs to update timeout limits based on the value of pretimeout) is very driver specific, and should be kept in the driver. I would prefer to keep it out of the API if possible.
Unless I am missing something, it should be possible to call the update_limits function in the driver after calling init_timeouts.
Thanks, Guenter
Hi Guenter,
Great thanks, I have got your review feedback, I will fix the problems :-) For update_limits, I also don't want to have that in the watchdog driver, and you can't find it in my last version. And this version, I integrate the watchdog_init_timeout and watchdog_init_pretimeout. so I can not add a driver specific "update_limits" between them. I think maybe we can not make this "update_limits" after calling init_timeouts, because we need to test and verify the timeout setting right after init_pretimeout by max_timeout and min_timeout. that is why I call update_limits right after init_pretimeout and before init_timeout.
any suggestion ? Great thanks ! :-)
On 21 May 2015 at 18:17, Guenter Roeck linux@roeck-us.net wrote:
On 05/21/2015 03:05 AM, Fu Wei wrote:
Hi Guenter,
Thanks for review. :-) feedback inline below
On 21 May 2015 at 17:04, Guenter Roeck linux@roeck-us.net wrote:
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org
[ ... ]
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
unsigned int pretimeout_parm,unsigned int timeout_parm,void (*update_limits)(structwatchdog_device *),
struct device *dev);-The watchdog_init_timeout function allows you to initialize the timeout field -using the module timeout parameter or by retrieving the timeout-sec property from -the device tree (if the module timeout parameter is invalid). Best practice is -to set the default timeout value as timeout value in the watchdog_device and -then use this function to set the user "preferred" timeout value. +The watchdog_init_timeouts function allows you to initialize the pretimeout and +timeout fields using the module pretimeout and timeout parameter or by +retrieving the elements in the timeout-sec property(the first element is for +timeout, the second one is for pretimeout) from the device tree(if the module +pretimeout and timeout parameter are invalid). +Normally, the pretimeout value will affect the limitation of timeout, and it +is also hardware related. So you can write a function in your driver to update +the limitation of timeout, according to the pretimeout value. Then pass the +function pointer by the update_limits parameter. If you driver doesn't +need this adjustment, just pass NULL to the update_limits parameter.
You've lost me a bit with the update_limits function. watchdog_init_timeouts() is called from the driver.
yes, that is the help function which will be called from watchdog driver, like SBSA watchdog driver
Why should the function have to call back into the driver to update the parameters which are passed from the driver ?
Let me explain this, please correct me if I misunderstand something. According to the concept of "pretimeout" in kernel, the timeout contains the pretimeout, like
- Kernel/API: P---------| pretimeout
|-------------------------------T timeoutIf you set up the value of pretimeout, that means pretimeout <min_timeout < timeout < max_timeout < (pretimeout + max_timeout_for_1th_stage) For min_timeout > pretimeout. if some one setup a timeout like : pretimeout > timeout > min_timeout, I think that could be a problem For max_timeout < (pretimeout + max_timeout_for_1th_stage), if some one setup a timeout like (pretimeout + max_timeout_for_1th_stage) < timeout > max_timeout .
I have explained a little in doc, but the adjustment may have something to do with hardware, like max_timeout_for_1th_stage(in SBSA watchdog , limited by WCV)
maybe this problem wouldn't happen ,if you set up max_timeout to a small number. so you can pass NULL to the pointer. but I think maybe for other device , that may happen.
Seems to me the driver can do that calculation first, then call watchdog_init_timeouts() with the result. Am I missing something ?
maybe I am overthinking it :-) please correct me
I just sent a more complete review. In general I think this problem (where the driver needs to update timeout limits based on the value of pretimeout) is very driver specific, and should be kept in the driver. I would prefer to keep it out of the API if possible.
Unless I am missing something, it should be possible to call the update_limits function in the driver after calling init_timeouts.
Thanks, Guenter
On 05/21/2015 03:50 AM, Fu Wei wrote:
Hi Guenter,
Great thanks, I have got your review feedback, I will fix the problems :-) For update_limits, I also don't want to have that in the watchdog driver, and you can't find it in my last version. And this version, I integrate the watchdog_init_timeout and watchdog_init_pretimeout. so I can not add a driver specific "update_limits" between them. I think maybe we can not make this "update_limits" after calling init_timeouts, because we need to test and verify the timeout setting right after init_pretimeout by max_timeout and min_timeout. that is
Don't you have to do that in set_pretimeout as well, and even adjust timeout if necessary ?
why I call update_limits right after init_pretimeout and before init_timeout.
any suggestion ? Great thanks ! :-)
Any sequence of set_timeout() set_pretimeout() can result in an invalid timeout, since the practical timeout limits changed. Therefore, you'll have to validate (and possibly update) the timeout after or during each call to set_pretimeout(). Changing min_timeout and max_timeout won't help there, that validation still has to happen.
Given this, some other driver developer may choose to not change the timeout limits at all, validate (and if necessary silently adjust) the timeout value in the set_timeout function, and call the set_timeout function from set_pretimeout to ensure that the timeout is still valid after the call to set_pretimeout. At least I would most likely implement it that way.
This means your choice of continuously updating the timeout limits is really a driver developer choice, not something that is needed in the driver core. It might even be problematic, since driver developers might forget to re-validate the current timeout after changing the pretimeout in a similar situation.
Having said that, and looking through the SBSA driver code, it seems to me that it does not re-validate the current timeout after setting a new pretimeout. Unless I am missing something, set_timeout(10); set_pretimeout(20); might therefore result in an invalid / unexpected timeout value. Setting set_timeout(10); set_pretimeout(10); might have even more interesting results. Can you try what happens with your driver if you set those values ?
Thanks, Guenter
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
drivers
Signed-off-by: Fu Wei fu.wei@linaro.org
Documentation/watchdog/watchdog-kernel-api.txt | 62 ++++++++++++--- drivers/watchdog/watchdog_core.c | 103 +++++++++++++++++++------ drivers/watchdog/watchdog_dev.c | 48 ++++++++++++ include/linux/watchdog.h | 30 ++++++- 4 files changed, 208 insertions(+), 35 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index a0438f3..43900df 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -49,6 +49,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout;
- unsigned int pretimeout;
- unsigned int min_pretimeout;
- unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status;
@@ -70,6 +73,9 @@ It contains following fields:
- timeout: the watchdog timer's timeout value (in seconds).
- min_timeout: the watchdog timer's minimum timeout value (in seconds).
- max_timeout: the watchdog timer's maximum timeout value (in seconds).
+* pretimeout: the watchdog timer's pretimeout value (in seconds). +* min_pretimeout: the watchdog timer's minimum pretimeout value (in seconds). +* max_pretimeout: the watchdog timer's maximum pretimeout value (in seconds).
- bootstatus: status of the device after booting (reported with watchdog WDIOF_* status bits).
- driver_data: a pointer to the drivers private data of a watchdog device.
@@ -92,6 +98,7 @@ struct watchdog_ops { int (*ping)(struct watchdog_device *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int);
- int (*set_pretimeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *);
@@ -153,9 +160,19 @@ they are supported. These optional routines/operations are: and -EIO for "could not write value to the watchdog". On success this routine should set the timeout value of the watchdog_device to the achieved timeout value (which may be different from the requested one
- because the watchdog does not necessarily has a 1 second resolution).
- because the watchdog does not necessarily has a 1 second resolution;
- If the driver supports pretimeout, then the timeout value must be greater
- than that). (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the watchdog's info structure).
+* set_pretimeout: this routine checks and changes the pretimeout of the
- watchdog timer device. It returns 0 on success, -EINVAL for "parameter out of
- range" and -EIO for "could not write value to the watchdog". On success this
- routine should set the pretimeout value of the watchdog_device to the
- achieved pretimeout value (which may be different from the requested one
- because the watchdog does not necessarily has a 1 second resolution).
- (Note: the WDIOF_PRETIMEOUT needs to be set in the options field of the
- watchdog's info structure).
allocated watchdog_device struct.
- get_timeleft: this routines returns the time that's left before a reset.
- ref: the operation that calls kref_get on the kref of a dynamically
@@ -213,14 +230,41 @@ The watchdog_get_drvdata function allows you to retrieve driver specific data. The argument of this function is the watchdog device where you want to retrieve data from. The function returns the pointer to the driver specific data.
-To initialize the timeout field, the following function can be used: +To initialize the timeout and pretimeout fields, the following function can be +used:
-extern int watchdog_init_timeout(struct watchdog_device *wdd,
unsigned int timeout_parm, struct device *dev);
I think this API should still be listed here. Drivers not supporting pretimeout can and should still use it.
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
unsigned int pretimeout_parm,unsigned int timeout_parm,void (*update_limits)(struct watchdog_device *),struct device *dev);-The watchdog_init_timeout function allows you to initialize the timeout field -using the module timeout parameter or by retrieving the timeout-sec property from -the device tree (if the module timeout parameter is invalid). Best practice is -to set the default timeout value as timeout value in the watchdog_device and -then use this function to set the user "preferred" timeout value. +The watchdog_init_timeouts function allows you to initialize the pretimeout and +timeout fields using the module pretimeout and timeout parameter or by +retrieving the elements in the timeout-sec property(the first element is for +timeout, the second one is for pretimeout) from the device tree(if the module +pretimeout and timeout parameter are invalid). +Normally, the pretimeout value will affect the limitation of timeout, and it +is also hardware related. So you can write a function in your driver to update +the limitation of timeout, according to the pretimeout value. Then pass the +function pointer by the update_limits parameter. If you driver doesn't +need this adjustment, just pass NULL to the update_limits parameter.
Is there a reason to believe that the update_limits function is necessary and can not be handled by the set_pretimeout and set_timeout driver functions, or possibly by the driver itself after calling watchdog_init_timeouts() ?
+Most of watchdog timers have not pretimeout as the warning signal. They just +reset the system, once the timeout watch period expires. In this case, we can +pass 0 to the pretimeout_parm, and pass NULL to the update_limits. Or use the +old API: watchdog_init_timeout(see below) +Best practice is to set the default pretimeout and timeout value as pretimeout +and timeout value in the watchdog_device and then use this function to set the +user "preferred" pretimeout value. This routine returns zero on success and a negative errno code for failure.
+For backward compatibility, we also support the timeout initialization API:
Why only for backward compatibility ? Why (try to) force new drivers which don't support pretimeout to use the new API function ?
+static inline int watchdog_init_timeout(struct watchdog_device *wdd,
unsigned int timeout_parm,struct device *dev);+Because of the introduction of pretimeout and watchdog_init_timeouts, this +function has become a small wrapper function of watchdog_init_timeouts.
The last sentence is irrelevant; how watchdog_init_timeout() is implemented is an implementation detail, not part of the API.
Empty lines at end of file will cause whitespace errors on merge. Doesn't checkpatch complain about this ?
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index cec9b55..460796e 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -43,60 +43,115 @@ static DEFINE_IDA(watchdog_ida); static struct class *watchdog_class;
-static void watchdog_check_min_max_timeout(struct watchdog_device *wdd) +static void watchdog_check_min_max_timeouts(struct watchdog_device *wdd) { /*
* Check that we have valid min and max timeout values, if* not reset them both to 0 (=not used or unknown)
* Check that we have valid min and max pretimeout and timeout values,* if not reset them both to 0 (=not used or unknown)- if (wdd->min_pretimeout > wdd->max_pretimeout) {
pr_info("Invalid min and max pretimeout, resetting to 0!\n");
Please drop the "!" at the end. While used in the old code, it is unnecessary and often frowned upon nowadays.
wdd->min_pretimeout = 0;wdd->max_pretimeout = 0;} if (wdd->min_timeout > wdd->max_timeout) { pr_info("Invalid min and max timeout values, resetting to 0!\n"); wdd->min_timeout = 0; wdd->max_timeout = 0; }
/*
* Check that we have valid min and max timeout values,* if not reset them both to pretimeout limits*/if (wdd->min_pretimeout && wdd->min_timeout < wdd->min_pretimeout) {
pr_info("Invalid min timeout, resetting to min pretimeout!\n");wdd->min_timeout = wdd->min_pretimeout;}
if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout) {
pr_info("Invalid max timeout, resetting to max pretimeout!\n");wdd->max_timeout = wdd->max_pretimeout;} }
/**
- watchdog_init_timeout() - initialize the timeout field
- watchdog_init_timeouts() - initialize the pretimeout and timeout field
- @pretimeout_parm: pretimeout module parameter
- @timeout_parm: timeout module parameter
- @dev: Device that stores the timeout-sec property
- Initialize the timeout field of the watchdog_device struct with either the
- timeout module parameter (if it is valid value) or the timeout-sec property
- (only if it is a valid value and the timeout_parm is out of bounds).
- If none of them are valid then we keep the old value (which should normally
- be the default timeout value.
- Initialize the pretimeout and timeout field of the watchdog_device struct
- with either the pretimeout and timeout module parameter (if it is valid
- value) or the timeout-sec property (only if it is a valid value and the
- pretimeout_parm and timeout_parm is out of bounds). If none of them are
- valid then we keep the old value (which should normally be the default
- timeout value.
valid, then we keep ... default timeout value).
- A zero is returned on success and -EINVAL for failure.
*/ -int watchdog_init_timeout(struct watchdog_device *wdd,
unsigned int timeout_parm, struct device *dev)+int watchdog_init_timeouts(struct watchdog_device *wdd,
unsigned int pretimeout_parm,unsigned int timeout_parm,void (*update_limits)(struct watchdog_device *),struct device *dev)
- unsigned int t = 0;
- unsigned int timeout = 0, pretimeout = 0;
- const __be32 *ppretimeout; int ret = 0;
- struct property *timeout_sec;
- int length;
- watchdog_check_min_max_timeout(wdd);
- watchdog_check_min_max_timeouts(wdd);
- /* try to get the timeout module parameter first */
- if (!watchdog_timeout_invalid(wdd, timeout_parm) && timeout_parm) {
wdd->timeout = timeout_parm;return ret;
- /* try to get the timeout and pretimeout module parameter first */
- if (pretimeout_parm) {
if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm)) {wdd->pretimeout = pretimeout_parm;if (update_limits)update_limits(wdd);} else {pr_warn("Invalid pretimeout parameter!\n");
We didn't have all this noise earlier. Can we keep it that way ?
ret = -EINVAL;}
- if (timeout_parm)
if (timeout_parm) {
if (!watchdog_timeout_invalid(wdd, timeout_parm)) {wdd->timeout = timeout_parm;return ret;}pr_warn("Invalid timeout parameter!\n");ret = -EINVAL;
}
/* try to get the timeout_sec property */ if (dev == NULL || dev->of_node == NULL) return ret;
- of_property_read_u32(dev->of_node, "timeout-sec", &t);
- if (!watchdog_timeout_invalid(wdd, t) && t)
wdd->timeout = t;- else
- timeout_sec = of_find_property(dev->of_node, "timeout-sec", &length);
- if (timeout_sec) {
ppretimeout = of_prop_next_u32(timeout_sec, NULL, &timeout);
Could you just use of_property_read_u32_array() and pass length as parameter ?
if (length == 2) {of_prop_next_u32(timeout_sec, ppretimeout, &pretimeout);if (!watchdog_pretimeout_invalid(wdd, pretimeout) &&pretimeout) {wdd->pretimeout = pretimeout;if (update_limits)update_limits(wdd);} else {ret = -EINVAL;}}if (!watchdog_timeout_invalid(wdd, timeout) && timeout)wdd->timeout = timeout;elseret = -EINVAL;} else { ret = -EINVAL;
}
return ret; }
-EXPORT_SYMBOL_GPL(watchdog_init_timeout); +EXPORT_SYMBOL_GPL(watchdog_init_timeouts);
/**
- watchdog_register_device() - register a watchdog device
@@ -119,7 +174,7 @@ int watchdog_register_device(struct watchdog_device *wdd) if (wdd->ops->start == NULL || wdd->ops->stop == NULL) return -EINVAL;
- watchdog_check_min_max_timeout(wdd);
watchdog_check_min_max_timeouts(wdd);
/*
- Note: now that all watchdog_device data has been verified, we
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 6aaefba..b519257 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -218,6 +218,38 @@ out_timeout: }
/*
- watchdog_set_pretimeout: set the watchdog timer pretimeout
- @wddev: the watchdog device to set the timeout for
- @pretimeout: pretimeout to set in seconds
- */
+static int watchdog_set_pretimeout(struct watchdog_device *wddev,
unsigned int pretimeout)+{
- int err;
- if (!wddev->ops->set_pretimeout ||
!(wddev->info->options & WDIOF_PRETIMEOUT))return -EOPNOTSUPP;- if (watchdog_pretimeout_invalid(wddev, pretimeout))
return -EINVAL;- mutex_lock(&wddev->lock);
- if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
err = -ENODEV;goto out_pretimeout;- }
- err = wddev->ops->set_pretimeout(wddev, pretimeout);
+out_pretimeout:
- mutex_unlock(&wddev->lock);
- return err;
+}
+/*
- watchdog_get_timeleft: wrapper to get the time left before a reboot
- @wddev: the watchdog device to get the remaining time from
- @timeleft: the time that's left
@@ -388,6 +420,22 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd, if (wdd->timeout == 0) return -EOPNOTSUPP; return put_user(wdd->timeout, p);
- case WDIOC_SETPRETIMEOUT:
if (get_user(val, p))return -EFAULT;err = watchdog_set_pretimeout(wdd, val);if (err < 0)return err;/* If the watchdog is active then we send a keepalive ping* to make sure that the watchdog keep's running (and if* possible that it takes the new timeout) */
Please use the standard multi-line comment format.
watchdog_ping(wdd);/* Fall */- case WDIOC_GETPRETIMEOUT:
/* timeout == 0 means that we don't use the pretimeout */
pretimeout == 0 means ...
"use" or "know" ?
if (wdd->pretimeout == 0)return -EOPNOTSUPP;return put_user(wdd->pretimeout, p);diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index a746bf5..df430a3 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -25,6 +25,7 @@ struct watchdog_device;
- @ping: The routine that sends a keepalive ping to the watchdog device.
- @status: The routine that shows the status of the watchdog device.
- @set_timeout:The routine for setting the watchdog devices timeout value.
- @set_pretimeout:The routine for setting the watchdog devices pretimeout value
- @get_timeleft:The routine that get's the time that's left before a reset.
- @ref: The ref operation for dyn. allocated watchdog_device structs
- @unref: The unref operation for dyn. allocated watchdog_device structs
@@ -44,6 +45,7 @@ struct watchdog_ops { int (*ping)(struct watchdog_device *); unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int);
- int (*set_pretimeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); void (*ref)(struct watchdog_device *); void (*unref)(struct watchdog_device *);
@@ -62,6 +64,9 @@ struct watchdog_ops {
- @timeout: The watchdog devices timeout value.
- @min_timeout:The watchdog devices minimum timeout value.
- @max_timeout:The watchdog devices maximum timeout value.
- @pretimeout: The watchdog devices pretimeout value.
- @min_pretimeout:The watchdog devices minimum pretimeout value.
- @max_pretimeout:The watchdog devices maximum pretimeout value.
- @driver-data:Pointer to the drivers private data.
- @lock: Lock for watchdog core internal use only.
- @status: Field that contains the devices internal status bits.
@@ -86,6 +91,9 @@ struct watchdog_device { unsigned int timeout; unsigned int min_timeout; unsigned int max_timeout;
- unsigned int pretimeout;
- unsigned int min_pretimeout;
- unsigned int max_pretimeout; void *driver_data; struct mutex lock; unsigned long status;
@@ -120,6 +128,14 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne (t < wdd->min_timeout || t > wdd->max_timeout)); }
+/* Use the following function to check if a pretimeout value is invalid */ +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
unsigned int t)+{
- return ((wdd->max_pretimeout != 0) &&
Please drop the unnecessary ( ).
(t < wdd->min_pretimeout || t > wdd->max_pretimeout));+}
- /* Use the following functions to manipulate watchdog driver specific data */ static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data) {
@@ -132,11 +148,21 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd) }
/* drivers/watchdog/watchdog_core.c */ -extern int watchdog_init_timeout(struct watchdog_device *wdd,
unsigned int timeout_parm, struct device *dev);+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
unsigned int pretimeout_parm,unsigned int timeout_parm,void (*update_limits)(struct watchdog_device *),struct device *dev);+static inline int watchdog_init_timeout(struct watchdog_device *wdd,
unsigned int timeout_parm,struct device *dev)+{
- return watchdog_init_timeouts(wdd, 0, timeout_parm, NULL, dev);
+}
- #ifdef CONFIG_HARDLOCKUP_DETECTOR void watchdog_nmi_disable_all(void); void watchdog_nmi_enable_all(void);
On Thu, May 21, 2015 at 04:32:34PM +0800, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org
[ ... ]
+/* Use the following function to check if a pretimeout value is invalid */ +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
unsigned int t)+{
- return ((wdd->max_pretimeout != 0) &&
(t < wdd->min_pretimeout || t > wdd->max_pretimeout));+}
Should this function also enforce "t < wdd->timeout", and should watchdog_timeout_invalid() enforce "t > wdd->pretimeout" ?
Thanks, Guenter
Hi Guenter,
On 21 May 2015 at 23:32, Guenter Roeck linux@roeck-us.net wrote:
On Thu, May 21, 2015 at 04:32:34PM +0800, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org
[ ... ]
+/* Use the following function to check if a pretimeout value is invalid */ +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
unsigned int t)+{
return ((wdd->max_pretimeout != 0) &&(t < wdd->min_pretimeout || t > wdd->max_pretimeout));+}
Should this function also enforce "t < wdd->timeout", and should watchdog_timeout_invalid() enforce "t > wdd->pretimeout" ?
yes, you are right , thanks for the correction !! :-)
Thanks, Guenter
Hi Guenter,
Great thanks for your suggestion,
I have put this kind of validation into watchdog_pretimeout_invalid and watchdog_timeout_invalid.
So :
(1) set_timeout(10); ------> if this setting is successful set_pretimeout(20); -----> return fail (-EINVAL)
(2) set_timeout(10); ------> if this setting is successful set_pretimeout(10); -----> return fail (-EINVAL)
this kind of situation will not result in an invalid / unexpected timeout value.
you will see this change in my next patchset
On 21 May 2015 at 23:32, Guenter Roeck linux@roeck-us.net wrote:
On Thu, May 21, 2015 at 04:32:34PM +0800, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Signed-off-by: Fu Wei fu.wei@linaro.org
[ ... ]
+/* Use the following function to check if a pretimeout value is invalid */ +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
unsigned int t)+{
return ((wdd->max_pretimeout != 0) &&(t < wdd->min_pretimeout || t > wdd->max_pretimeout));+}
Should this function also enforce "t < wdd->timeout", and should watchdog_timeout_invalid() enforce "t > wdd->pretimeout" ?
Thanks, Guenter
On 21.05.2015 11:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Hi,
As I was proposing some other API changes with my early-timeout-sec work, I can see my work is going to collide with your API change proposal a bit. So maybe I should ask your opinion as well..
Could this pretimeout feature be something that other drivers could benefit too? I can see that it does not do anything else except call panic() before letting the watchdog expire. This is something that could be emulated easily by the watchdog core for drivers that don't know anything about pretimeouts at all.
The way I was planning the API change there would need to be a small change with each watchdog driver in order to let the watchdog core take over generic behaviour on behalf of the driver. My goal was to make the change so that each driver that gets converted to the new API extensions gets a support for early-timeout-sec for free, without needing to enable support for it any way. If the API was designed properly, also pretimeouts could be handled easily and maybe even so that other drivers could have that feature even though their hardware does not explicitly give any support for it.
Any thoughts?
Thanks, -Timo
Hi Timo,
On 22 May 2015 at 14:30, Timo Kokkonen timo.kokkonen@offcode.fi wrote:
On 21.05.2015 11:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Hi,
As I was proposing some other API changes with my early-timeout-sec work, I can see my work is going to collide with your API change proposal a bit. So maybe I should ask your opinion as well..
Thank you for reminding me of that, I saw "early-timeout-sec", but because I don't get it in kernel API or watchdog_core.c, so I did not pay attention to it. Sorry.
Could this pretimeout feature be something that other drivers could benefit too?
yes , as you may know, I am making a patch for pretimeout support in watchdog framework
I can see that it does not do anything else except call panic() before letting the watchdog expire. This is something that could be emulated easily by the watchdog core for drivers that don't know anything about pretimeouts at all.
For SBSA watchdog, there are two stage timeout, and according to kernel doc: ---------------------- Pretimeouts:
Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets. ----------------------
I think panic() is the right way to do now. but people may also need to config : PANIC_TIMEOUT [!=0] KEXEC KDUMP for debug reason
The way I was planning the API change there would need to be a small change with each watchdog driver in order to let the watchdog core take over generic behaviour on behalf of the driver. My goal was to make the change so that each driver that gets converted to the new API extensions gets a support for early-timeout-sec for free, without needing to enable support for it any way. If the API was designed properly, also pretimeouts could be handled easily and maybe even so that other drivers could have that feature even though their hardware does not explicitly give any support for it.
could you please point out your patch , then I can learn your idea :-) For now , I am working on a common "Pretimeouts" following the concept in kernel doc.
Any thoughts?
my thoughts is in my pretimeout patch , would you provide some suggestion ?
Great thanks !
Thanks, -Timo
On 22.05.2015 11:23, Fu Wei wrote:
Hi Timo,
On 22 May 2015 at 14:30, Timo Kokkonen timo.kokkonen@offcode.fi wrote:
On 21.05.2015 11:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Hi,
As I was proposing some other API changes with my early-timeout-sec work, I can see my work is going to collide with your API change proposal a bit. So maybe I should ask your opinion as well..
Thank you for reminding me of that, I saw "early-timeout-sec", but because I don't get it in kernel API or watchdog_core.c, so I did not pay attention to it. Sorry.
Could this pretimeout feature be something that other drivers could benefit too?
yes , as you may know, I am making a patch for pretimeout support in watchdog framework
I can see that it does not do anything else except call panic() before letting the watchdog expire. This is something that could be emulated easily by the watchdog core for drivers that don't know anything about pretimeouts at all.
For SBSA watchdog, there are two stage timeout, and according to kernel doc:
Pretimeouts:
Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets.
I think panic() is the right way to do now. but people may also need to config : PANIC_TIMEOUT [!=0] KEXEC KDUMP for debug reason
Yes, so basically if we hit pretimeout, we probably have already crashed. The only difference is that we now have some seconds time to dump out useful data and then either reboot or let the actual watchdog reset take care of resetting the device. panic() sounds like a good thing to do. Maybe you could also dump registers on other CPUs too or try to get out some other useful information about the kernel in case it has crashed or something. But I'm just guessing.
The way I was planning the API change there would need to be a small change with each watchdog driver in order to let the watchdog core take over generic behaviour on behalf of the driver. My goal was to make the change so that each driver that gets converted to the new API extensions gets a support for early-timeout-sec for free, without needing to enable support for it any way. If the API was designed properly, also pretimeouts could be handled easily and maybe even so that other drivers could have that feature even though their hardware does not explicitly give any support for it.
could you please point out your patch , then I can learn your idea :-) For now , I am working on a common "Pretimeouts" following the concept in kernel doc.
Any thoughts?
my thoughts is in my pretimeout patch , would you provide some suggestion ?
Here is an archive link to my patch set:
http://www.spinics.net/lists/linux-watchdog/msg06473.html
Your patch set is adding a new call to the core for adjusting the pretimoeut, which is probably a good thing in case the driver needs special handling for it anyway. But if the core had capability to emulate pretimeout for drivers that can't support it in hardware, it would be good if there was a way for the core to support it even though the driver had zero code for it. The core also has watchdog_init_timeout() right now but even that is not called from that many drivers. I would like to fix that too so that drivers would not need to bother so much about it but let core take care of it more. This is why I proposed the watchdog_init_params() call that could dig all the generic parameters from device tree on behalf of the driver. This is where I though timeout and early-timeout-sec parameters would be parsed from device tree, but it could also parse pretimeout parameter as well. Apparently Guenter did not like my approach so we might need some other way to do it.
I don't get to say how this should be done, I'm just throwing ideas here. But I think the watchdog core would be a lot better place for implementing the generic features than drivers. That way a lot of drivers could enable support the new features for free.
Thanks, -Timo
Hi Timo,
On 22 May 2015 at 16:59, Timo Kokkonen timo.kokkonen@offcode.fi wrote:
On 22.05.2015 11:23, Fu Wei wrote:
Hi Timo,
On 22 May 2015 at 14:30, Timo Kokkonen timo.kokkonen@offcode.fi wrote:
On 21.05.2015 11:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Hi,
As I was proposing some other API changes with my early-timeout-sec work, I can see my work is going to collide with your API change proposal a bit. So maybe I should ask your opinion as well..
Thank you for reminding me of that, I saw "early-timeout-sec", but because I don't get it in kernel API or watchdog_core.c, so I did not pay attention to it. Sorry.
Could this pretimeout feature be something that other drivers could benefit too?
yes , as you may know, I am making a patch for pretimeout support in watchdog framework
I can see that it does not do anything else except call panic() before letting the watchdog expire. This is something that could be emulated easily by the watchdog core for drivers that don't know anything about pretimeouts at all.
For SBSA watchdog, there are two stage timeout, and according to kernel doc:
Pretimeouts:
Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets.
I think panic() is the right way to do now. but people may also need to config : PANIC_TIMEOUT [!=0] KEXEC KDUMP for debug reason
Yes, so basically if we hit pretimeout, we probably have already crashed.
yes, for now , I only use panic(), but at the beginning, I offer user some options:
https://lists.linaro.org/pipermail/linaro-acpi/2015-April/004350.html
The only difference is that we now have some seconds time to dump out useful data and then either reboot or let the actual watchdog reset take care of resetting the device. panic() sounds like a good thing to do. Maybe you could also dump registers on other CPUs too or try to get out some other useful information about the kernel in case it has crashed or something. But I'm just guessing.
yes, that is my thought. because there is the kdump support in panic(), so that can give use a chance to figure out why the watchdog wasn't fed.
The way I was planning the API change there would need to be a small change with each watchdog driver in order to let the watchdog core take over generic behaviour on behalf of the driver. My goal was to make the change so that each driver that gets converted to the new API extensions gets a support for early-timeout-sec for free, without needing to enable support for it any way. If the API was designed properly, also pretimeouts could be handled easily and maybe even so that other drivers could have that feature even though their hardware does not explicitly give any support for it.
could you please point out your patch , then I can learn your idea :-) For now , I am working on a common "Pretimeouts" following the concept in kernel doc.
Any thoughts?
my thoughts is in my pretimeout patch , would you provide some suggestion ?
Here is an archive link to my patch set:
Ah , cool, I can see some common in your patch. Thanks :-) See if I can learn something from your patches
Your patch set is adding a new call to the core for adjusting the pretimoeut, which is probably a good thing in case the driver needs special handling for it anyway. But if the core had capability to emulate pretimeout for drivers that can't support it in hardware, it would be good if there was a way for the core to support it even though the driver had zero code for it. The core also has watchdog_init_timeout() right now but even that is not called from that many drivers. I would like to fix that too so that drivers would not need to bother so much about it but let core take care of it more. This is why I proposed the watchdog_init_params() call that could dig all the generic parameters from device tree on behalf of the driver. This is where I though timeout and early-timeout-sec parameters would be parsed from device tree, but it could also parse pretimeout parameter as well. Apparently Guenter did not like my approach so we might need some other way to do it.
I am using pretimeout, because SBSA watchdog hardware support two stages timeout, I am reusing the existing kernel concept, but your early_timeout_sec is a new concept. If you check my previous patchset , you will see : at the beginning, pretimeout support is not a generic features in my patchset. Then Arnd suggest that I can try to make pretimeout into watchdog framework, and Guenter said :"Makes sense." So I am still trying to improve pretimeout support :-) If I can make pretimeout merged, may be you can try pretimeout to implement early_timeout_sec function? It is up to the maintainers, I will try my best.
Thanks :-)
I don't get to say how this should be done, I'm just throwing ideas here. But I think the watchdog core would be a lot better place for implementing the generic features than drivers. That way a lot of drivers could enable support the new features for free.
Thanks, -Timo
On 22.05.2015 13:46, Fu Wei wrote:
Hi Timo,
On 22 May 2015 at 16:59, Timo Kokkonen timo.kokkonen@offcode.fi wrote:
On 22.05.2015 11:23, Fu Wei wrote:
Hi Timo,
On 22 May 2015 at 14:30, Timo Kokkonen timo.kokkonen@offcode.fi wrote:
On 21.05.2015 11:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Also update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts".
Reasons: (1)kernel already has two watchdog drivers are using "pretimeout": drivers/char/ipmi/ipmi_watchdog.c drivers/watchdog/kempld_wdt.c(but the definition is different) (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
Hi,
As I was proposing some other API changes with my early-timeout-sec work, I can see my work is going to collide with your API change proposal a bit. So maybe I should ask your opinion as well..
Thank you for reminding me of that, I saw "early-timeout-sec", but because I don't get it in kernel API or watchdog_core.c, so I did not pay attention to it. Sorry.
Could this pretimeout feature be something that other drivers could benefit too?
yes , as you may know, I am making a patch for pretimeout support in watchdog framework
I can see that it does not do anything else except call panic() before letting the watchdog expire. This is something that could be emulated easily by the watchdog core for drivers that don't know anything about pretimeouts at all.
For SBSA watchdog, there are two stage timeout, and according to kernel doc:
Pretimeouts:
Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets.
I think panic() is the right way to do now. but people may also need to config : PANIC_TIMEOUT [!=0] KEXEC KDUMP for debug reason
Yes, so basically if we hit pretimeout, we probably have already crashed.
yes, for now , I only use panic(), but at the beginning, I offer user some options:
https://lists.linaro.org/pipermail/linaro-acpi/2015-April/004350.html
The only difference is that we now have some seconds time to dump out useful data and then either reboot or let the actual watchdog reset take care of resetting the device. panic() sounds like a good thing to do. Maybe you could also dump registers on other CPUs too or try to get out some other useful information about the kernel in case it has crashed or something. But I'm just guessing.
yes, that is my thought. because there is the kdump support in panic(), so that can give use a chance to figure out why the watchdog wasn't fed.
Yes indeed, sounds good!
The way I was planning the API change there would need to be a small change with each watchdog driver in order to let the watchdog core take over generic behaviour on behalf of the driver. My goal was to make the change so that each driver that gets converted to the new API extensions gets a support for early-timeout-sec for free, without needing to enable support for it any way. If the API was designed properly, also pretimeouts could be handled easily and maybe even so that other drivers could have that feature even though their hardware does not explicitly give any support for it.
could you please point out your patch , then I can learn your idea :-) For now , I am working on a common "Pretimeouts" following the concept in kernel doc.
Any thoughts?
my thoughts is in my pretimeout patch , would you provide some suggestion ?
Here is an archive link to my patch set:
Ah , cool, I can see some common in your patch. Thanks :-) See if I can learn something from your patches
Your patch set is adding a new call to the core for adjusting the pretimoeut, which is probably a good thing in case the driver needs special handling for it anyway. But if the core had capability to emulate pretimeout for drivers that can't support it in hardware, it would be good if there was a way for the core to support it even though the driver had zero code for it. The core also has watchdog_init_timeout() right now but even that is not called from that many drivers. I would like to fix that too so that drivers would not need to bother so much about it but let core take care of it more. This is why I proposed the watchdog_init_params() call that could dig all the generic parameters from device tree on behalf of the driver. This is where I though timeout and early-timeout-sec parameters would be parsed from device tree, but it could also parse pretimeout parameter as well. Apparently Guenter did not like my approach so we might need some other way to do it.
I am using pretimeout, because SBSA watchdog hardware support two stages timeout, I am reusing the existing kernel concept, but your early_timeout_sec is a new concept.
Yes, the early-timeout-sec concept itself is new in mainline but there are a lot of production devices out there that start up with watchdog running and the watchdog must not be stopped ever. Those devices typically hack the watchdog policy somehow into the drivers so that they don't just freeze even if kernel or early userspace happens to crash for some reason. So I thought it would be about time to get support for this in mainline. I don't want to hack this feature any more into drivers :)
If you check my previous patchset , you will see : at the beginning, pretimeout support is not a generic features in my patchset. Then Arnd suggest that I can try to make pretimeout into watchdog framework, and Guenter said :"Makes sense."
Yeah, the same story as mine. Early-timeout-sec was first atmel driver specific but I was told to think something more generic. At first I objected as I felt I would need to implement a lot of stuff into the core before I could make it generic. Then I was given chance to spend some time with this and I actually wrote a generic implementation in the core in a way that allows all drivers to use it with minimal modifications. It also lets core deal with unstoppable watchdogs and watchdogs with very short maximum timeout so that driver don't need special handling for it.
So I am still trying to improve pretimeout support :-) If I can make pretimeout merged, may be you can try pretimeout to implement early_timeout_sec function? It is up to the maintainers, I will try my best.
Yes, which brings us to the actual implementation details. Right now we only have watchdog_init_timeout() that some drivers are using. Your proposal is going into direction where we would also have init_watchdog_pretimeout() or init_watchdog_timeouts() call. Drivers providing pretimeout support would call these functions to set up pretimeout values.
My patch set is taking the timeout concept into completely different direction where we have hardware timeout properties and then we have userspace timeout requests. Watchdog core is in the middle and checks whether userspace timeout requested by user is something small enough that the hardware can actually support. If not, a worker pings the watchdog core on behalf of userspace so that user daemon can have longer timeout. The same works with unstoppable watchdogs and the new early-timeout-sec property. Right now drivers with restricted hardware are just implementing this on their own.
Also the parameter parsing is different on my patch set. I was trying to make it so that all generic parameters were parsed by the core and driver would not need to bother about then, unless user had specified eg. module parameter that it fills in the watchdog_device structure and the core would not then override it. This way new parameters could be introduced in a way that the driver does not explicitly need to implement support for it if core knows how to handle it. There was someone making windowed watchdogs, maybe we could implement support for that in core and let the watchdog core emulate windowed watchdog support for all drivers even if the actual hardware implementation was not there.
This is the direction we could go eventually. Of course not all features are in interest for everyone and it may not make sense to try implement everything in the core. But we have a lot of drivers that implement all sorts of timers in them and my patch set could eliminate a lot of code from them. We also have couple of drivers that have pretimeout support and there seems to be a desire make that generic as well. Obviously I think it makes sense to combine this effort in order to avoid conflicts.
So I am too hoping more guidelines on what is the acceptable direction where to aim at. For example we could make this parameter handling more generic and future proof if we allow an API change that require small change to all drivers. I don't know exactly where the line is drawn.
Thanks, -Timo
On 05/22/2015 05:14 AM, Timo Kokkonen wrote: [ ... ]
This is the direction we could go eventually. Of course not all features are in interest for everyone and it may not make sense to try implement everything in the core. But we have a lot of drivers that implement all sorts of timers in them and my patch set could eliminate a lot of code from them. We also have couple of drivers that have pretimeout support and there seems to be a desire make that generic as well. Obviously I think it makes sense to combine this effort in order to avoid conflicts.
I would suggest to avoid trying to combine efforts. If anything, I would suggest to _separate_ patch sets.
So I am too hoping more guidelines on what is the acceptable direction where to aim at. For example we could make this parameter handling more generic and future proof if we allow an API change that require small change to all drivers. I don't know exactly where the line is drawn.
As I just mentioned separately, the more changes you make, the less likely it will be to get your changes accepted. A patch set requiring changes to all drivers would have to provide substantial benefits to get accepted. Talking about parameters (and assuming you mean module parameters), I don't even see how you could make that work without changing the userspace ABI, which would be a no-go.
Personally I think it would be easier if you tried to accomplish less than more. Concentrate on early-timeout (only), get it merged. Provide a clear separation between is_active (as in "watchdog running") from is_open (userspace has the driver open) and get it merged. Add the ability to "ping" a running watchdog while the driver is closed into the watchdog core and get it merged. Add the ability to soft-ping a watchdog while the driver is open into the core and get it merged. Those are all logically separate functions, and could be introduced separately.
Just my personal opinion, of course.
Thanks, Guenter
On 05/22/2015 03:46 AM, Fu Wei wrote:
Hi Timo,
[ ... ]
So I am still trying to improve pretimeout support :-)
Is there anything still missing from it ?
If I can make pretimeout merged, may be you can try pretimeout to implement early_timeout_sec function?
Not sure how one would or even could do that.
Do you mean "implement early_pretimeout_sec", by any chance ?
It is up to the maintainers, I will try my best.
Please don't make the pretimeout concept more complicated than necessary.
The smaller the patch, the more likely it is to get accepted. The more you change, the more difficult it is for the maintainer to, for example, back-port later bug fixes into earlier kernel releases when needed. This is why it is, for example, better to keep the existing watchdog_init_timeout() function instead of just replacing it with watchdog_init_timeouts().
Try to put yourself into the maintainer's perspective: If you were the maintainer, would you rather accept a patch or patch set which maintains the existing API and doesn't require any changes to existing drivers, or would you accept one that changes, say, some function or variable names and will require manual back-ports later on if there is a bug fix ? Would you rather accept a patch that adds 50 lines of code, or one that changes another 100+ lines and rearranges everything along the line ?
Thanks, Guenter
Hi Guenter.
Sorry for my poor English . let me explain this :
On 22 May 2015 at 21:23, Guenter Roeck linux@roeck-us.net wrote:
On 05/22/2015 03:46 AM, Fu Wei wrote:
Hi Timo,
[ ... ]
So I am still trying to improve pretimeout support :-)
Is there anything still missing from it ?
If I can make pretimeout merged, may be you can try pretimeout to implement early_timeout_sec function?
Not sure how one would or even could do that.
Do you mean "implement early_pretimeout_sec", by any chance ?
I mean: using pretimeout to implement the function you want, instead of early_pretimeout_sec
Hope I say the right word this time :-)
It is up to the maintainers, I will try my best.
Please don't make the pretimeout concept more complicated than necessary.
The smaller the patch, the more likely it is to get accepted. The more you change, the more difficult it is for the maintainer to, for example, back-port later bug fixes into earlier kernel releases when needed. This is why it is, for example, better to keep the existing watchdog_init_timeout() function instead of just replacing it with watchdog_init_timeouts().
Try to put yourself into the maintainer's perspective: If you were the maintainer, would you rather accept a patch or patch set which maintains the existing API and doesn't require any changes to existing drivers, or would you accept one that changes, say, some function or variable names and will require manual back-ports later on if there is a bug fix ? Would you rather accept a patch that adds 50 lines of code, or one that changes another 100+ lines and rearranges everything along the line ?
Thanks, Guenter
On Fri, May 22, 2015 at 10:38:32PM +0800, Fu Wei wrote:
Hi Guenter.
Sorry for my poor English . let me explain this :
On 22 May 2015 at 21:23, Guenter Roeck linux@roeck-us.net wrote:
On 05/22/2015 03:46 AM, Fu Wei wrote:
Hi Timo,
[ ... ]
So I am still trying to improve pretimeout support :-)
Is there anything still missing from it ?
If I can make pretimeout merged, may be you can try pretimeout to implement early_timeout_sec function?
Not sure how one would or even could do that.
Do you mean "implement early_pretimeout_sec", by any chance ?
I mean: using pretimeout to implement the function you want, instead of early_pretimeout_sec
How would this work if the watchdog hardware doesn't support pretimeout ?
Pretimeout and early timeout are two logically different functions, with different goals, so I don't entirely (if at all) understand why it would make sense to tie them together.
Can you elaborate why you think this would be a good idea ?
Thanks, Guenter
Hi Guenter,
On 22 May 2015 at 23:05, Guenter Roeck linux@roeck-us.net wrote:
On Fri, May 22, 2015 at 10:38:32PM +0800, Fu Wei wrote:
Hi Guenter.
Sorry for my poor English . let me explain this :
On 22 May 2015 at 21:23, Guenter Roeck linux@roeck-us.net wrote:
On 05/22/2015 03:46 AM, Fu Wei wrote:
Hi Timo,
[ ... ]
So I am still trying to improve pretimeout support :-)
Is there anything still missing from it ?
If I can make pretimeout merged, may be you can try pretimeout to implement early_timeout_sec function?
Not sure how one would or even could do that.
Do you mean "implement early_pretimeout_sec", by any chance ?
I mean: using pretimeout to implement the function you want, instead of early_pretimeout_sec
How would this work if the watchdog hardware doesn't support pretimeout ?
Pretimeout and early timeout are two logically different functions, with different goals, so I don't entirely (if at all) understand why it would make sense to tie them together.
Can you elaborate why you think this would be a good idea ?
sorry, my apology. forget about this, :-) I think we should focus on SBSA watchdog patch here, but not a early_timeout_sec.
Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Fu Wei fu.wei@linaro.org
This driver bases on linux kernel watchdog framework, and use "pretimeout" in the framework. It supports getting timeout and pretimeout from parameter and FDT at the driver init stage. In first timeout(WS0), the interrupt routine run panic to save system context.
Signed-off-by: Fu Wei fu.wei@linaro.org --- drivers/watchdog/Kconfig | 12 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TEST + depends on ARM_ARCH_TIMER + select WATCHDOG_CORE + help + ARM SBSA Generic Watchdog timer. This has two Watchdog Signals + (WS0/WS1), will trigger a warning interrupt(do panic) at the first + timeout(WS0); will reboot your system when the second timeout(WS1) + is reached. + More details: DEN0029B - Server Base System Architecture (SBSA) + config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..471f1b7c 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_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c new file mode 100644 index 0000000..4ebe7c3 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,476 @@ +/* + * 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 + * + * 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. + * + * Note: This SBSA Generic watchdog driver is compatible with + * the pretimeout concept of Linux kernel. + * The timeout and pretimeout are set by the different REGs. + * The first watch period is set by writing WCV directly, + * that can support more than 10s timeout at the maximum + * system counter frequency. + * The second watch period is set by WOR(32bit) which will be loaded + * automatically by hardware, when WS0 is triggered. + * This gives a maximum watch period of around 10s at the maximum + * system counter frequency. + * The System Counter shall run at maximum of 400MHz. + * More details: DEN0029B - Server Base System Architecture (SBSA) + * + * Kernel/API: P---------| pretimeout + * |-------------------------------T timeout + * SBSA GWDT: P--WOR---WS1 pretimeout + * |-------WCV----------WS0~~~~~~~~T timeout + */ + +#include <asm/arch_timer.h> + +#include <linux/acpi.h> +#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> + +/* 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_LO 0x010 +#define SBSA_GWDT_WCV_HI 0x014 + +/* 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 10 /* seconds, the 1st + 2nd watch periods*/ +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/ + +static unsigned int timeout; +module_param(timeout, uint, 0); +MODULE_PARM_DESC(timeout, + "Watchdog timeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_TIMEOUT) ")"); + +static unsigned int max_timeout = UINT_MAX; +module_param(max_timeout, uint, 0); +MODULE_PARM_DESC(max_timeout, + "Watchdog max timeout in seconds. (>=0, default=" + __MODULE_STRING(UINT_MAX) ")"); + +static unsigned int pretimeout; +module_param(pretimeout, uint, 0); +MODULE_PARM_DESC(pretimeout, + "Watchdog pretimeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_PRETIMEOUT) ")"); + +static unsigned int max_pretimeout = U32_MAX; +module_param(max_pretimeout, uint, 0); +MODULE_PARM_DESC(max_pretimeout, + "Watchdog max pretimeout in seconds. (>=0, default=" + __MODULE_STRING(U32_MAX) ")"); + +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) ")"); + +/* + * Architected system timer support. + */ +static void sbsa_gwdt_cf_write(unsigned int reg, u32 val, + struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + writel_relaxed(val, gwdt->control_base + reg); +} + +static void sbsa_gwdt_rf_write(unsigned int reg, u32 val, + struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + writel_relaxed(val, gwdt->refresh_base + reg); +} + +static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + + return readl_relaxed(gwdt->control_base + reg); +} + +/* + * help founctions for accessing 64bit WCV register + */ +static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) +{ + u32 wcv_lo, wcv_hi; + + do { + wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd); + wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd); + } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd)); + + return (((u64)wcv_hi << 32) | wcv_lo); +} + +static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) +{ + u32 wcv_lo, wcv_hi; + + wcv_lo = value & U32_MAX; + wcv_hi = (value >> 32) & U32_MAX; + + sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd); + sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd); +} + +static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 wcv; + + wcv = arch_counter_get_cntvct() + + (u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk; + + sbsa_gwdt_set_wcv(wdd, wcv); +} + +/* + * Use the following function to set the limit of timeout + * after updating pretimeout + */ +static void sbsa_gwdt_update_limits(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 first_period_max = U64_MAX; + + do_div(first_period_max, gwdt->clk); + + wdd->min_timeout = wdd->pretimeout + 1; + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), + wdd->max_timeout); +} + +static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + wdd->timeout = timeout; + + return 0; +} + +static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd, + unsigned int pretimeout) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u32 wor; + + wdd->pretimeout = pretimeout; + sbsa_gwdt_update_limits(wdd); + + if (!pretimeout) + /* gives sbsa_gwdt_start a chance to setup timeout */ + wor = gwdt->clk; + else + wor = pretimeout * gwdt->clk; + + /* refresh the WOR, that will cause an explicit watchdog refresh */ + sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd); + + return 0; +} + +static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{ + struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd); + u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct(); + + do_div(timeleft, gwdt->clk); + + return timeleft; +} + +static int sbsa_gwdt_start(struct watchdog_device *wdd) +{ + /* Force refresh */ + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); + /* writing WCS will cause an explicit watchdog refresh */ + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, SBSA_GWDT_WCS_EN, wdd); + + reload_timeout_to_wcv(wdd); + + return 0; +} + +static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{ + /* Force refresh */ + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); + /* writing WCS will cause an explicit watchdog refresh */ + sbsa_gwdt_cf_write(SBSA_GWDT_WCS, 0, wdd); + + return 0; +} + +static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{ + /* + * Writing WRR for an explicit watchdog refresh + * You can write anyting(like 0xc0ffee) + */ + sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd); + + reload_timeout_to_wcv(wdd); + + return 0; +} + +static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{ + struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id; + struct watchdog_device *wdd = &gwdt->wdd; + u32 status; + + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); + + if (status & SBSA_GWDT_WCS_WS0) + panic("SBSA Watchdog pre-timeout"); + + return IRQ_HANDLED; +} + +static struct watchdog_info sbsa_gwdt_info = { + .identity = "SBSA Generic Watchdog", + .options = WDIOF_SETTIMEOUT | + WDIOF_KEEPALIVEPING | + WDIOF_MAGICCLOSE | + WDIOF_PRETIMEOUT | + WDIOF_CARDRESET, +}; + +static struct watchdog_ops sbsa_gwdt_ops = { + .owner = THIS_MODULE, + .start = sbsa_gwdt_start, + .stop = sbsa_gwdt_stop, + .ping = sbsa_gwdt_keepalive, + .set_timeout = sbsa_gwdt_set_timeout, + .set_pretimeout = sbsa_gwdt_set_pretimeout, + .get_timeleft = sbsa_gwdt_get_timeleft, +}; + +static int sbsa_gwdt_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct sbsa_gwdt *gwdt; + struct watchdog_device *wdd; + struct resource *res; + void *rf_base, *cf_base; + int irq; + u32 clk, status; + int ret = 0; + u64 first_period_max = U64_MAX; + + /* + * Try to determine the frequency from the arch_timer interface + */ + clk = arch_timer_get_rate(); + if (!clk) { + dev_err(dev, "System Counter frequency not available\n"); + return -EINVAL; + } + + gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL); + if (!gwdt) + return -ENOMEM; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "refresh"); + rf_base = devm_ioremap_resource(dev, res); + if (IS_ERR(rf_base)) + return PTR_ERR(rf_base); + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control"); + cf_base = devm_ioremap_resource(dev, res); + if (IS_ERR(rf_base)) + return PTR_ERR(rf_base); + + irq = platform_get_irq_byname(pdev, "ws0"); + if (irq < 0) { + dev_err(dev, "unable to get ws0 interrupt.\n"); + return irq; + } + + gwdt->refresh_base = rf_base; + gwdt->control_base = cf_base; + gwdt->clk = clk; + + platform_set_drvdata(pdev, gwdt); + + 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); + + status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd); + if (status & SBSA_GWDT_WCS_WS1) { + dev_warn(dev, "System reset by WDT(WCS: %x, WCV: %llx)\n", + status, sbsa_gwdt_get_wcv(wdd)); + wdd->bootstatus |= WDIOF_CARDRESET; + } + /* Check if watchdog is already enabled */ + if (status & SBSA_GWDT_WCS_EN) { + dev_warn(dev, "already enabled! WCS:0x%x\n", status); + sbsa_gwdt_keepalive(wdd); + } + + wdd->min_pretimeout = 0; + wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout); + wdd->min_timeout = 1; + do_div(first_period_max, clk); + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), + max_timeout); + + wdd->pretimeout = DEFAULT_PRETIMEOUT; + wdd->timeout = DEFAULT_TIMEOUT; + watchdog_init_timeouts(wdd, pretimeout, timeout, + sbsa_gwdt_update_limits, dev); + /* update pretimeout to WOR */ + sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd); + + ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, IRQF_TIMER, + pdev->name, gwdt); + if (ret) { + dev_err(dev, "unable to request IRQ %d\n", irq); + return ret; + } + + ret = watchdog_register_device(wdd); + if (ret) + return ret; + + dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ %uHz\n", + wdd->timeout, wdd->pretimeout, gwdt->clk); + + 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); + int ret = 0; + + if (!nowayout) + ret = sbsa_gwdt_stop(&gwdt->wdd); + + watchdog_unregister_device(&gwdt->wdd); + + return ret; +} + +/* 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_VERSION("v1.0"); +MODULE_AUTHOR("Fu Wei fu.wei@linaro.org"); +MODULE_AUTHOR("Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com"); +MODULE_LICENSE("GPL v2");
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This driver bases on linux kernel watchdog framework, and use "pretimeout" in the framework. It supports getting timeout and pretimeout from parameter and FDT at the driver init stage. In first timeout(WS0), the interrupt routine run panic to save system context.
Signed-off-by: Fu Wei fu.wei@linaro.org
drivers/watchdog/Kconfig | 12 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TEST
- depends on ARM_ARCH_TIMER
- select WATCHDOG_CORE
- help
ARM SBSA Generic Watchdog timer. This has two Watchdog Signals(WS0/WS1), will trigger a warning interrupt(do panic) at the firsttimeout(WS0); will reboot your system when the second timeout(WS1)is reached.
I think it would be better to keep the WS0/WS1 out of the help text. It is an implementation detail, and no user will be able to make sense of it.
More details: DEN0029B - Server Base System Architecture (SBSA)- config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..471f1b7c 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_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c new file mode 100644 index 0000000..4ebe7c3 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,476 @@ +/*
- 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>
- 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.
- Note: This SBSA Generic watchdog driver is compatible with
the pretimeout concept of Linux kernel.
The timeout and pretimeout are set by the different REGs.
The first watch period is set by writing WCV directly,
that can support more than 10s timeout at the maximum
system counter frequency.
The second watch period is set by WOR(32bit) which will be loaded
automatically by hardware, when WS0 is triggered.
This gives a maximum watch period of around 10s at the maximum
system counter frequency.
The System Counter shall run at maximum of 400MHz.
More details: DEN0029B - Server Base System Architecture (SBSA)
- Kernel/API: P---------| pretimeout
|-------------------------------T timeout
- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout- */
+#include <asm/arch_timer.h>
+#include <linux/acpi.h> +#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>
+/* 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_LO 0x010 +#define SBSA_GWDT_WCV_HI 0x014
+/* 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 10 /* seconds, the 1st + 2nd watch periods*/ +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/
+static unsigned int timeout; +module_param(timeout, uint, 0); +MODULE_PARM_DESC(timeout,
"Watchdog timeout in seconds. (>=0, default="__MODULE_STRING(DEFAULT_TIMEOUT) ")");+static unsigned int max_timeout = UINT_MAX; +module_param(max_timeout, uint, 0); +MODULE_PARM_DESC(max_timeout,
"Watchdog max timeout in seconds. (>=0, default="__MODULE_STRING(UINT_MAX) ")");+static unsigned int pretimeout; +module_param(pretimeout, uint, 0); +MODULE_PARM_DESC(pretimeout,
"Watchdog pretimeout in seconds. (>=0, default="__MODULE_STRING(DEFAULT_PRETIMEOUT) ")");+static unsigned int max_pretimeout = U32_MAX; +module_param(max_pretimeout, uint, 0); +MODULE_PARM_DESC(max_pretimeout,
"Watchdog max pretimeout in seconds. (>=0, default="__MODULE_STRING(U32_MAX) ")");+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) ")");+/*
- Architected system timer support.
Architected ? Sounds a bit odd. Not sure if this comment serves a real purpose.
- */
+static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
struct watchdog_device *wdd)+{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- writel_relaxed(val, gwdt->control_base + reg);
+}
+static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
struct watchdog_device *wdd)+{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- writel_relaxed(val, gwdt->refresh_base + reg);
+}
+static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- return readl_relaxed(gwdt->control_base + reg);
+}
+/*
- help founctions for accessing 64bit WCV register
functions
- */
+static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) +{
- u32 wcv_lo, wcv_hi;
- do {
wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd);wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd);- } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd));
- return (((u64)wcv_hi << 32) | wcv_lo);
+}
+static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) +{
- u32 wcv_lo, wcv_hi;
- wcv_lo = value & U32_MAX;
- wcv_hi = (value >> 32) & U32_MAX;
- sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd);
- sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd);
+}
+static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 wcv;
- wcv = arch_counter_get_cntvct() +
(u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;- sbsa_gwdt_set_wcv(wdd, wcv);
+}
+/*
- Use the following function to set the limit of timeout
s/limit of timeout/timeout limits/
- after updating pretimeout
- */
+static void sbsa_gwdt_update_limits(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 first_period_max = U64_MAX;
- do_div(first_period_max, gwdt->clk);
- wdd->min_timeout = wdd->pretimeout + 1;
- wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),
wdd->max_timeout);+}
+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)+{
- wdd->timeout = timeout;
- return 0;
+}
+static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd,
unsigned int pretimeout)+{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u32 wor;
- wdd->pretimeout = pretimeout;
- sbsa_gwdt_update_limits(wdd);
- if (!pretimeout)
/* gives sbsa_gwdt_start a chance to setup timeout */wor = gwdt->clk;- else
wor = pretimeout * gwdt->clk;- /* refresh the WOR, that will cause an explicit watchdog refresh */
- sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd);
- return 0;
+}
+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct();
Still not happy about the use of arch_counter_get_cntvct instead of using the clock subsystem. I am quite sure this could be done, possibly through arch_sys_counter, though at this point I am getting wary of bringing it up, so I guess I'll just let it go.
- do_div(timeleft, gwdt->clk);
- return timeleft;
+}
+static int sbsa_gwdt_start(struct watchdog_device *wdd) +{
- /* Force refresh */
- sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
- /* writing WCS will cause an explicit watchdog refresh */
- sbsa_gwdt_cf_write(SBSA_GWDT_WCS, SBSA_GWDT_WCS_EN, wdd);
- reload_timeout_to_wcv(wdd);
- return 0;
+}
+static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{
- /* Force refresh */
- sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
- /* writing WCS will cause an explicit watchdog refresh */
- sbsa_gwdt_cf_write(SBSA_GWDT_WCS, 0, wdd);
- return 0;
+}
+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{
- /*
* Writing WRR for an explicit watchdog refresh* You can write anyting(like 0xc0ffee)*/- sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
- reload_timeout_to_wcv(wdd);
- return 0;
+}
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
- struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
- struct watchdog_device *wdd = &gwdt->wdd;
- u32 status;
- status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
- if (status & SBSA_GWDT_WCS_WS0)
panic("SBSA Watchdog pre-timeout");- return IRQ_HANDLED;
+}
+static struct watchdog_info sbsa_gwdt_info = {
- .identity = "SBSA Generic Watchdog",
- .options = WDIOF_SETTIMEOUT |
WDIOF_KEEPALIVEPING |WDIOF_MAGICCLOSE |WDIOF_PRETIMEOUT |WDIOF_CARDRESET,+};
+static struct watchdog_ops sbsa_gwdt_ops = {
- .owner = THIS_MODULE,
- .start = sbsa_gwdt_start,
- .stop = sbsa_gwdt_stop,
- .ping = sbsa_gwdt_keepalive,
- .set_timeout = sbsa_gwdt_set_timeout,
- .set_pretimeout = sbsa_gwdt_set_pretimeout,
- .get_timeleft = sbsa_gwdt_get_timeleft,
+};
+static int sbsa_gwdt_probe(struct platform_device *pdev) +{
- struct device *dev = &pdev->dev;
- struct sbsa_gwdt *gwdt;
- struct watchdog_device *wdd;
- struct resource *res;
- void *rf_base, *cf_base;
- int irq;
- u32 clk, status;
- int ret = 0;
- u64 first_period_max = U64_MAX;
- /*
* Try to determine the frequency from the arch_timer interface*/- clk = arch_timer_get_rate();
arch_timer_get_rate() does not seem to be exported. Did you try to build the driver as module ?
- if (!clk) {
dev_err(dev, "System Counter frequency not available\n");return -EINVAL;- }
- gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
- if (!gwdt)
return -ENOMEM;- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "refresh");
- rf_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(rf_base))
return PTR_ERR(rf_base);- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
- cf_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(rf_base))
return PTR_ERR(rf_base);- irq = platform_get_irq_byname(pdev, "ws0");
- if (irq < 0) {
dev_err(dev, "unable to get ws0 interrupt.\n");return irq;- }
- gwdt->refresh_base = rf_base;
- gwdt->control_base = cf_base;
- gwdt->clk = clk;
- platform_set_drvdata(pdev, gwdt);
- 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);
- status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
- if (status & SBSA_GWDT_WCS_WS1) {
dev_warn(dev, "System reset by WDT(WCS: %x, WCV: %llx)\n",status, sbsa_gwdt_get_wcv(wdd));wdd->bootstatus |= WDIOF_CARDRESET;- }
- /* Check if watchdog is already enabled */
- if (status & SBSA_GWDT_WCS_EN) {
dev_warn(dev, "already enabled! WCS:0x%x\n", status);sbsa_gwdt_keepalive(wdd);- }
- wdd->min_pretimeout = 0;
- wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout);
- wdd->min_timeout = 1;
- do_div(first_period_max, clk);
- wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),
max_timeout);- wdd->pretimeout = DEFAULT_PRETIMEOUT;
- wdd->timeout = DEFAULT_TIMEOUT;
- watchdog_init_timeouts(wdd, pretimeout, timeout,
sbsa_gwdt_update_limits, dev);- /* update pretimeout to WOR */
- sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd);
- ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, IRQF_TIMER,
pdev->name, gwdt);- if (ret) {
dev_err(dev, "unable to request IRQ %d\n", irq);return ret;- }
- ret = watchdog_register_device(wdd);
- if (ret)
return ret;- dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ %uHz\n",
wdd->timeout, wdd->pretimeout, gwdt->clk);- 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);
- int ret = 0;
- if (!nowayout)
ret = sbsa_gwdt_stop(&gwdt->wdd);- watchdog_unregister_device(&gwdt->wdd);
- return ret;
+}
+/* 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_VERSION("v1.0"); +MODULE_AUTHOR("Fu Wei fu.wei@linaro.org"); +MODULE_AUTHOR("Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com"); +MODULE_LICENSE("GPL v2");
Hi Guenter,
Thanks for review :-)
On 21 May 2015 at 18:34, Guenter Roeck linux@roeck-us.net wrote:
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This driver bases on linux kernel watchdog framework, and use "pretimeout" in the framework. It supports getting timeout and pretimeout from parameter and FDT at the driver init stage. In first timeout(WS0), the interrupt routine run panic to save system context.
Signed-off-by: Fu Wei fu.wei@linaro.org
drivers/watchdog/Kconfig | 12 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTdepends on ARM_ARCH_TIMERselect WATCHDOG_COREhelpARM SBSA Generic Watchdog timer. This has two Watchdog Signals(WS0/WS1), will trigger a warning interrupt(do panic) at thefirst
timeout(WS0); will reboot your system when the secondtimeout(WS1)
is reached.I think it would be better to keep the WS0/WS1 out of the help text. It is an implementation detail, and no user will be able to make sense of it.
I don't mind to delete it , but those are in SBSA spec, is that an implementation detail ?
Sorry , just try to re-confirm it :-)
More details: DEN0029B - Server Base System Architecture (SBSA)- config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..471f1b7c 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_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c new file mode 100644 index 0000000..4ebe7c3 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,476 @@ +/*
- 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>
- 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.
- Note: This SBSA Generic watchdog driver is compatible with
the pretimeout concept of Linux kernel.
The timeout and pretimeout are set by the different REGs.
The first watch period is set by writing WCV directly,
that can support more than 10s timeout at the maximum
system counter frequency.
The second watch period is set by WOR(32bit) which will beloaded
automatically by hardware, when WS0 is triggered.
This gives a maximum watch period of around 10s at the maximum
system counter frequency.
The System Counter shall run at maximum of 400MHz.
More details: DEN0029B - Server Base System Architecture (SBSA)
- Kernel/API: P---------| pretimeout
|-------------------------------T timeout
- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout- */
+#include <asm/arch_timer.h>
+#include <linux/acpi.h> +#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>
+/* 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_LO 0x010 +#define SBSA_GWDT_WCV_HI 0x014
+/* 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 10 /* seconds, the 1st + 2nd watch periods*/ +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/
+static unsigned int timeout; +module_param(timeout, uint, 0); +MODULE_PARM_DESC(timeout,
"Watchdog timeout in seconds. (>=0, default="__MODULE_STRING(DEFAULT_TIMEOUT) ")");+static unsigned int max_timeout = UINT_MAX; +module_param(max_timeout, uint, 0); +MODULE_PARM_DESC(max_timeout,
"Watchdog max timeout in seconds. (>=0, default="__MODULE_STRING(UINT_MAX) ")");+static unsigned int pretimeout; +module_param(pretimeout, uint, 0); +MODULE_PARM_DESC(pretimeout,
"Watchdog pretimeout in seconds. (>=0, default="__MODULE_STRING(DEFAULT_PRETIMEOUT) ")");+static unsigned int max_pretimeout = U32_MAX; +module_param(max_pretimeout, uint, 0); +MODULE_PARM_DESC(max_pretimeout,
"Watchdog max pretimeout in seconds. (>=0, default="__MODULE_STRING(U32_MAX) ")");+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) ")");+/*
- Architected system timer support.
Architected ? Sounds a bit odd. Not sure if this comment serves a real purpose.
Ah, sorry , this is comment is a legacy , will update it . :-)
- */
+static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
struct watchdog_device *wdd)+{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);writel_relaxed(val, gwdt->control_base + reg);+}
+static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
struct watchdog_device *wdd)+{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);writel_relaxed(val, gwdt->refresh_base + reg);+}
+static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device *wdd) +{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);return readl_relaxed(gwdt->control_base + reg);+}
+/*
- help founctions for accessing 64bit WCV register
functions
Ah , typo , sorry
- */
+static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) +{
u32 wcv_lo, wcv_hi;do {wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd);wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd);} while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd));return (((u64)wcv_hi << 32) | wcv_lo);+}
+static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) +{
u32 wcv_lo, wcv_hi;wcv_lo = value & U32_MAX;wcv_hi = (value >> 32) & U32_MAX;sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd);sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd);+}
+static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);u64 wcv;wcv = arch_counter_get_cntvct() +(u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;sbsa_gwdt_set_wcv(wdd, wcv);+}
+/*
- Use the following function to set the limit of timeout
s/limit of timeout/timeout limits/
Thanks , will fix it
- after updating pretimeout
- */
+static void sbsa_gwdt_update_limits(struct watchdog_device *wdd) +{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);u64 first_period_max = U64_MAX;do_div(first_period_max, gwdt->clk);wdd->min_timeout = wdd->pretimeout + 1;wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),wdd->max_timeout);+}
+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)+{
wdd->timeout = timeout;return 0;+}
+static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd,
unsigned int pretimeout)+{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);u32 wor;wdd->pretimeout = pretimeout;sbsa_gwdt_update_limits(wdd);if (!pretimeout)/* gives sbsa_gwdt_start a chance to setup timeout */wor = gwdt->clk;elsewor = pretimeout * gwdt->clk;/* refresh the WOR, that will cause an explicit watchdog refresh*/
sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd);return 0;+}
+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct();Still not happy about the use of arch_counter_get_cntvct instead of using the clock subsystem. I am quite sure this could be done, possibly through arch_sys_counter, though at this point I am getting wary of bringing it up, so I guess I'll just let it go.
yes, It is the code I should check again, will sent more feedback once I get better idea
do_div(timeleft, gwdt->clk);return timeleft;+}
+static int sbsa_gwdt_start(struct watchdog_device *wdd) +{
/* Force refresh */sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);/* writing WCS will cause an explicit watchdog refresh */sbsa_gwdt_cf_write(SBSA_GWDT_WCS, SBSA_GWDT_WCS_EN, wdd);reload_timeout_to_wcv(wdd);return 0;+}
+static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{
/* Force refresh */sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);/* writing WCS will cause an explicit watchdog refresh */sbsa_gwdt_cf_write(SBSA_GWDT_WCS, 0, wdd);return 0;+}
+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{
/** Writing WRR for an explicit watchdog refresh* You can write anyting(like 0xc0ffee)*/sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);reload_timeout_to_wcv(wdd);return 0;+}
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;u32 status;status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);if (status & SBSA_GWDT_WCS_WS0)panic("SBSA Watchdog pre-timeout");return IRQ_HANDLED;+}
+static struct watchdog_info sbsa_gwdt_info = {
.identity = "SBSA Generic Watchdog",.options = WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING |WDIOF_MAGICCLOSE |WDIOF_PRETIMEOUT |WDIOF_CARDRESET,+};
+static struct watchdog_ops sbsa_gwdt_ops = {
.owner = THIS_MODULE,.start = sbsa_gwdt_start,.stop = sbsa_gwdt_stop,.ping = sbsa_gwdt_keepalive,.set_timeout = sbsa_gwdt_set_timeout,.set_pretimeout = sbsa_gwdt_set_pretimeout,.get_timeleft = sbsa_gwdt_get_timeleft,+};
+static int sbsa_gwdt_probe(struct platform_device *pdev) +{
struct device *dev = &pdev->dev;struct sbsa_gwdt *gwdt;struct watchdog_device *wdd;struct resource *res;void *rf_base, *cf_base;int irq;u32 clk, status;int ret = 0;u64 first_period_max = U64_MAX;/** Try to determine the frequency from the arch_timer interface*/clk = arch_timer_get_rate();arch_timer_get_rate() does not seem to be exported. Did you try to build the driver as module ?
yes, I have built it as a ko module, that is why I made a patch to export this interface in the first patch of this patchset
but I will confirm it again :-)
if (!clk) {dev_err(dev, "System Counter frequency not available\n");return -EINVAL;}gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);if (!gwdt)return -ENOMEM;res = platform_get_resource_byname(pdev, IORESOURCE_MEM,"refresh");
rf_base = devm_ioremap_resource(dev, res);if (IS_ERR(rf_base))return PTR_ERR(rf_base);res = platform_get_resource_byname(pdev, IORESOURCE_MEM,"control");
cf_base = devm_ioremap_resource(dev, res);if (IS_ERR(rf_base))return PTR_ERR(rf_base);irq = platform_get_irq_byname(pdev, "ws0");if (irq < 0) {dev_err(dev, "unable to get ws0 interrupt.\n");return irq;}gwdt->refresh_base = rf_base;gwdt->control_base = cf_base;gwdt->clk = clk;platform_set_drvdata(pdev, gwdt);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);status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);if (status & SBSA_GWDT_WCS_WS1) {dev_warn(dev, "System reset by WDT(WCS: %x, WCV: %llx)\n",status, sbsa_gwdt_get_wcv(wdd));wdd->bootstatus |= WDIOF_CARDRESET;}/* Check if watchdog is already enabled */if (status & SBSA_GWDT_WCS_EN) {dev_warn(dev, "already enabled! WCS:0x%x\n", status);sbsa_gwdt_keepalive(wdd);}wdd->min_pretimeout = 0;wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout);wdd->min_timeout = 1;do_div(first_period_max, clk);wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),max_timeout);wdd->pretimeout = DEFAULT_PRETIMEOUT;wdd->timeout = DEFAULT_TIMEOUT;watchdog_init_timeouts(wdd, pretimeout, timeout,sbsa_gwdt_update_limits, dev);/* update pretimeout to WOR */sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd);ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, IRQF_TIMER,pdev->name, gwdt);if (ret) {dev_err(dev, "unable to request IRQ %d\n", irq);return ret;}ret = watchdog_register_device(wdd);if (ret)return ret;dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @%uHz\n",
wdd->timeout, wdd->pretimeout, gwdt->clk);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);int ret = 0;if (!nowayout)ret = sbsa_gwdt_stop(&gwdt->wdd);watchdog_unregister_device(&gwdt->wdd);return ret;+}
+/* 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_VERSION("v1.0"); +MODULE_AUTHOR("Fu Wei fu.wei@linaro.org"); +MODULE_AUTHOR("Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com"); +MODULE_LICENSE("GPL v2");
On Thu, May 21, 2015 at 07:08:34PM +0800, Fu Wei wrote:
Hi Guenter,
Thanks for review :-)
On 21 May 2015 at 18:34, Guenter Roeck linux@roeck-us.net wrote:
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This driver bases on linux kernel watchdog framework, and use "pretimeout" in the framework. It supports getting timeout and pretimeout from parameter and FDT at the driver init stage. In first timeout(WS0), the interrupt routine run panic to save system context.
Signed-off-by: Fu Wei fu.wei@linaro.org
drivers/watchdog/Kconfig | 12 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTdepends on ARM_ARCH_TIMERselect WATCHDOG_COREhelpARM SBSA Generic Watchdog timer. This has two Watchdog Signals(WS0/WS1), will trigger a warning interrupt(do panic) at thefirst
timeout(WS0); will reboot your system when the secondtimeout(WS1)
is reached.I think it would be better to keep the WS0/WS1 out of the help text. It is an implementation detail, and no user will be able to make sense of it.
I don't mind to delete it , but those are in SBSA spec, is that an implementation detail ?
I think so.
Ask yourself - assuming you are not involved in the development of this driver, and you read this help text. Is the help text going to help you to know about WS0 and WS1 ? Why not just something like the following, if you want to be verbose ?
ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts. The first timeout will trigger a panic; the second timeout will trigger a system reset.
Anyway, not worth arguing about.
/** Try to determine the frequency from the arch_timer interface*/clk = arch_timer_get_rate();arch_timer_get_rate() does not seem to be exported. Did you try to build the driver as module ?
yes, I have built it as a ko module, that is why I made a patch to export this interface in the first patch of this patchset
but I will confirm it again :-)
Guess I'll give it a try myself. I don't really understand how this can work unless arch_timer_get_rate() is exported in your tree.
Guenter
Hi Guenter,
Great thanks :-)
On 21 May 2015 at 23:18, Guenter Roeck linux@roeck-us.net wrote:
On Thu, May 21, 2015 at 07:08:34PM +0800, Fu Wei wrote:
Hi Guenter,
Thanks for review :-)
On 21 May 2015 at 18:34, Guenter Roeck linux@roeck-us.net wrote:
On 05/21/2015 01:32 AM, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This driver bases on linux kernel watchdog framework, and use "pretimeout" in the framework. It supports getting timeout and pretimeout from parameter and FDT at the driver init stage. In first timeout(WS0), the interrupt routine run panic to save system context.
Signed-off-by: Fu Wei fu.wei@linaro.org
drivers/watchdog/Kconfig | 12 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTdepends on ARM_ARCH_TIMERselect WATCHDOG_COREhelpARM SBSA Generic Watchdog timer. This has two Watchdog Signals(WS0/WS1), will trigger a warning interrupt(do panic) at thefirst
timeout(WS0); will reboot your system when the secondtimeout(WS1)
is reached.I think it would be better to keep the WS0/WS1 out of the help text. It is an implementation detail, and no user will be able to make sense of it.
I don't mind to delete it , but those are in SBSA spec, is that an implementation detail ?
I think so.
Ask yourself - assuming you are not involved in the development of this driver, and you read this help text. Is the help text going to help you to know about WS0 and WS1 ? Why not just something like the following, if you want to be verbose ?
ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts. The first timeout will trigger a panic; the second timeout will trigger a system reset.
Great thanks, that is better, yes, If I never read SBSA spec, I don't know WS is watchdog signal :-)
Anyway, not worth arguing about.
yes, agree , Thanks for the example "help" :-)
/** Try to determine the frequency from the arch_timer interface*/clk = arch_timer_get_rate();arch_timer_get_rate() does not seem to be exported. Did you try to build the driver as module ?
yes, I have built it as a ko module, that is why I made a patch to export this interface in the first patch of this patchset
but I will confirm it again :-)
Guess I'll give it a try myself. I don't really understand how this can work unless arch_timer_get_rate() is exported in your tree.
yes, I have exported it , I think it make sense to export it. Because other driver maybe need to get system counter info
Do you agree ? :-)
Guenter
On Thu, May 21, 2015 at 11:46:53PM +0800, Fu Wei wrote:
Hi Guenter,
[ ... ]
/** Try to determine the frequency from the arch_timer interface*/clk = arch_timer_get_rate();arch_timer_get_rate() does not seem to be exported. Did you try to build the driver as module ?
yes, I have built it as a ko module, that is why I made a patch to export this interface in the first patch of this patchset
but I will confirm it again :-)
Guess I'll give it a try myself. I don't really understand how this can work unless arch_timer_get_rate() is exported in your tree.
yes, I have exported it , I think it make sense to export it. Because other driver maybe need to get system counter info
Do you agree ? :-)
I don't think it is for me to agree or not. The arm maintainers will need to be involved. You can not just export such a function without maintainer Ack.
Having said that, my personal preference would be for the counter and rate to be exported through the clock subsystem (ie with clk_get_rate). But that would still not provide the current counter value, so maybe that isn't even possible.
Thanks, Guenter
Hi Guenter,
On 21 May 2015 at 23:59, Guenter Roeck linux@roeck-us.net wrote:
On Thu, May 21, 2015 at 11:46:53PM +0800, Fu Wei wrote:
Hi Guenter,
[ ... ]
/** Try to determine the frequency from the arch_timer interface*/clk = arch_timer_get_rate();arch_timer_get_rate() does not seem to be exported. Did you try to build the driver as module ?
yes, I have built it as a ko module, that is why I made a patch to export this interface in the first patch of this patchset
but I will confirm it again :-)
Guess I'll give it a try myself. I don't really understand how this can work unless arch_timer_get_rate() is exported in your tree.
yes, I have exported it , I think it make sense to export it. Because other driver maybe need to get system counter info
Do you agree ? :-)
I don't think it is for me to agree or not. The arm maintainers will need to be involved. You can not just export such a function without maintainer Ack.
yes, I have added Mark Rutland in the CC list when I sent this patchset . He added the "arch_timer_get_rate();" in the arch timer driver. See if he can provide some suggestion. :-)
maybe I should add some more arm arch timer maintainers to get more help on this :-)
Having said that, my personal preference would be for the counter and rate to be exported through the clock subsystem (ie with clk_get_rate). But that would still not provide the current counter value, so maybe that isn't even possible.
I will try to make a patch for this, If the arm maintainers don't like exporting "arch_timer_get_rate();"
But your thought is good, the clk_get_rate is the best way to do for now
Thanks, Guenter
On 05/21/2015 11:12 AM, Fu Wei wrote:
Having said that, my personal preference would be for the counter and rate to be exported through the clock subsystem (ie with clk_get_rate). But that would still not provide the current counter value, so maybe that isn't even possible.
I will try to make a patch for this, If the arm maintainers don't like exporting "arch_timer_get_rate();"
But your thought is good, the clk_get_rate is the best way to do for now
The rate isn't the problem. It's the current timestamp counter. The only way to get that is with either arch_counter_get_cntvct() or arch_timer_read_counter(). I'm not sure which of the two functions is better. However, arch_timer_read_counter() is really just a function pointer that points to arch_counter_get_cntvct().
Also, clk_get_rate() only works if you have a real clk object. I've said this before many times, but on my ACPI platform, there are no clk objects. Clocks are handled by UEFI.
Hi Timur.
On 22 May 2015 at 00:33, Timur Tabi timur@codeaurora.org wrote:
On 05/21/2015 11:12 AM, Fu Wei wrote:
Having said that, my personal preference would be for the counter and rate to be exported through the clock subsystem (ie with clk_get_rate). But that would still not provide the current counter value, so maybe that isn't even possible.
I will try to make a patch for this, If the arm maintainers don't like exporting "arch_timer_get_rate();"
But your thought is good, the clk_get_rate is the best way to do for now
The rate isn't the problem. It's the current timestamp counter. The only way to get that is with either arch_counter_get_cntvct() or arch_timer_read_counter(). I'm not sure which of the two functions is better. However, arch_timer_read_counter() is really just a function pointer that points to arch_counter_get_cntvct().
Also, clk_get_rate() only works if you have a real clk object. I've said this before many times, but on my ACPI platform, there are no clk objects. Clocks are handled by UEFI.
Thanks for your info, I think : right now , do what we can do, export the interface of arch_timer, If the arm maintainers don't like this way, we can find another way later or find a option as plan B.
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Guenter Roeck wrote:
+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct();
Still not happy about the use of arch_counter_get_cntvct instead of using the clock subsystem. I am quite sure this could be done, possibly through arch_sys_counter, though at this point I am getting wary of bringing it up, so I guess I'll just let it go.
You made the same comment with my driver, and I keep asking for clarification. The clk_get_sys() API does not work on my system, because there are not clocks defined. That must be an ACPI limitation that I can't fix.
The alternative to arch_counter_get_cntvct() is arch_timer_read_counter(), which is not exported. So we have two choices,
1) Continue to use arch_counter_get_cntvct(), which works on all ARM64 platforms that this driver supports anyway
2) Export arch_timer_read_counter()
I prefer option #1.
On Thu, May 21, 2015 at 08:09:02AM -0500, Timur Tabi wrote:
Guenter Roeck wrote:
+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct();
Still not happy about the use of arch_counter_get_cntvct instead of using the clock subsystem. I am quite sure this could be done, possibly through arch_sys_counter, though at this point I am getting wary of bringing it up, so I guess I'll just let it go.
You made the same comment with my driver, and I keep asking for clarification. The clk_get_sys() API does not work on my system, because there are not clocks defined. That must be an ACPI limitation that I can't fix.
Would it be possible to define such clocks ?
The alternative to arch_counter_get_cntvct() is arch_timer_read_counter(), which is not exported. So we have two choices,
- Continue to use arch_counter_get_cntvct(), which works on all ARM64
platforms that this driver supports anyway
- Export arch_timer_read_counter()
I prefer option #1.
Do we have any feedback from the arm maintainers ?
My problem is that I don't want to be the first one to permit using those functions outside architecture and clock code. If we do this, we should get an Ack from an arm maintainer specifically for the use of arch_counter_get_cntvct() and arch_timer_get_rate().
Thanks, Guenter
Hi Guenter,
On 21 May 2015 at 23:28, Guenter Roeck linux@roeck-us.net wrote:
On Thu, May 21, 2015 at 08:09:02AM -0500, Timur Tabi wrote:
Guenter Roeck wrote:
+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct();
Still not happy about the use of arch_counter_get_cntvct instead of using the clock subsystem. I am quite sure this could be done, possibly through arch_sys_counter, though at this point I am getting wary of bringing it up, so I guess I'll just let it go.
You made the same comment with my driver, and I keep asking for clarification. The clk_get_sys() API does not work on my system, because there are not clocks defined. That must be an ACPI limitation that I can't fix.
Would it be possible to define such clocks ?
The alternative to arch_counter_get_cntvct() is arch_timer_read_counter(), which is not exported. So we have two choices,
- Continue to use arch_counter_get_cntvct(), which works on all ARM64
platforms that this driver supports anyway
- Export arch_timer_read_counter()
I prefer option #1.
Do we have any feedback from the arm maintainers ?
My problem is that I don't want to be the first one to permit using those functions outside architecture and clock code. If we do this, we should get an Ack from an arm maintainer specifically for the use of arch_counter_get_cntvct() and arch_timer_get_rate().
IMO, we may need to use arch_timer_read_counter. Reason: Once the Linux kernel has KVM support and is in hyp mode. we may need to use arch_counter_get_cntpct. So arch_timer driver have helped us to make arch_timer_read_counter point to the right function. and arch_timer_read_counter is in the include/clocksource/arm_arch_timer.h, just like arch_timer_get_rate as a interface
Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fu Wei wrote:
Once the Linux kernel has KVM support and is in hyp mode. we may need to use arch_counter_get_cntpct.
arch_counter_get_cntpct() does not appear to be valid for ARM64:
http://lxr.free-electrons.com/source/arch/arm64/include/asm/arch_timer.h#L10...
Hi Timur.
On 25 May 2015 at 11:46, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
Once the Linux kernel has KVM support and is in hyp mode. we may need to use arch_counter_get_cntpct.
arch_counter_get_cntpct() does not appear to be valid for ARM64:
http://lxr.free-electrons.com/source/arch/arm64/include/asm/arch_timer.h#L10...
Great thank for correction, that make sense.
-- 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 05/21/2015 03:32 AM, fu.wei@linaro.org wrote:
+static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 wcv;
- wcv = arch_counter_get_cntvct() +
(u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;- sbsa_gwdt_set_wcv(wdd, wcv);
+}
...
+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)+{
- wdd->timeout = timeout;
- return 0;
+}
...
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
- struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
- struct watchdog_device *wdd = &gwdt->wdd;
- u32 status;
- status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
- if (status & SBSA_GWDT_WCS_WS0)
panic("SBSA Watchdog pre-timeout");- return IRQ_HANDLED;
+}
There's one thing I don't understand about your driver. The 'timeout' value from the kernel is supposed to to be the number of seconds until the system reboots. You are programming the WCV with that value, which means that the WS0 interrupt will fire when the timeout expires the first time. However, you don't reboot the system during this interrupt. The "panic" will cause the system to halt, but not reboot. Instead, it will just sit there. You're waiting for the WS1 timeout for the system to reboot, but this is not a clean reboot, and it occurs at 2*timeout seconds.
That's why I like my driver better. It doesn't have any of this pretimeout stuff, and when the timeout expires during the WS0 interrupt, it calls emergency_restart() which reboots the system properly. The WS1 hard reset is used as a "backup" reset in case emergency_restart() fails.
Hi Timur,
On 21 May 2015 at 23:42, Timur Tabi timur@codeaurora.org wrote:
On 05/21/2015 03:32 AM, fu.wei@linaro.org wrote:
+static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);u64 wcv;wcv = arch_counter_get_cntvct() +(u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;sbsa_gwdt_set_wcv(wdd, wcv);+}
...
+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)+{
wdd->timeout = timeout;return 0;+}
...
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;u32 status;status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);if (status & SBSA_GWDT_WCS_WS0)panic("SBSA Watchdog pre-timeout");return IRQ_HANDLED;+}
There's one thing I don't understand about your driver. The 'timeout' value from the kernel is supposed to to be the number of seconds until the system reboots. You are programming the WCV with that value, which means that the WS0 interrupt will fire when the timeout expires the first time. However, you don't reboot the system during this interrupt. The "panic" will cause the system to halt, but not reboot. Instead, it will just sit there.
the "panic" is not just halt the system, please check the code : (1)It can trigger Kdump (not just print the panic message), if you enable this in the config. that can help server administrator to figure out why the system goes wrong. (2)panic also can trigger a reboot, if you set up "panic timeout".
Obviously, it won't just sit there, it can help user figure out the problem.
At the beginning, I would like to make the first signal more useful, but for simplifying the first version of driver , I decide to use panic(). but if there is better "alerts" for a ARM server , I will go on maintaining this driver to make WS0 more useful.
You're waiting for the WS1 timeout for the system to reboot, but this is not a clean reboot, and it occurs at 2*timeout seconds.
That's why I like my driver better. It doesn't have any of this pretimeout stuff, and when the timeout expires during the WS0 interrupt, it calls emergency_restart() which reboots the system properly. The WS1 hard reset is used as a "backup" reset in case emergency_restart() fails.
OK, If you think so, I hope you can read the SBSA spec more carefully For the watchdog signal (WS0/WS1), SBSA say: "The initial signal is typically wired to an interrupt and alerts the system. The system can attempt to take corrective action that includes refreshing the watchdog within the second watch period. If the refresh is successful the system returns to the previous normal operation. If it fails then the second watch period expires and a second signal is generated. The signal is fed to a higher agent as an interrupt or reset for it to take executive action."
So WS0 is a warning, but not a reset. the WS1 maybe a reset, or a interrupt to higher agent.
That is different from a normal watchdog use before. the two stage of WS are not just for reset , at least the first one is definitely not a reset. and the second one is not a backup.
If you make SBSA watchdog work like a normal watchdog,: (1)why we need a new driver and new device? you can just use SP805 in the system. (2) why we need a two stages? ( if the second hardware reset signal can work more reliably , why use emergency_restart() which is a software reset, does it clean the system and do some useful backup or sync? ) the only useful thing done by emergency_restart() is kmsg_dump(KMSG_DUMP_EMERG);) (3)why the first WS is connect to a interrupt, but not a reset signal(I believe the direct reset signal is far more reliable than a interrupt to trigger a software reset )
And because of WS0 is a warning, so I decide to use a existing watchdog concept "pretimeout": ----------------- Pretimeouts:
Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets. -----------------
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Timur,
On 24 May 2015 at 00:28, Fu Wei fu.wei@linaro.org wrote:
Hi Timur,
On 21 May 2015 at 23:42, Timur Tabi timur@codeaurora.org wrote:
On 05/21/2015 03:32 AM, fu.wei@linaro.org wrote:
+static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);u64 wcv;wcv = arch_counter_get_cntvct() +(u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;sbsa_gwdt_set_wcv(wdd, wcv);+}
...
+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)+{
wdd->timeout = timeout;return 0;+}
...
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;u32 status;status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);if (status & SBSA_GWDT_WCS_WS0)panic("SBSA Watchdog pre-timeout");return IRQ_HANDLED;+}
There's one thing I don't understand about your driver. The 'timeout' value from the kernel is supposed to to be the number of seconds until the system reboots. You are programming the WCV with that value, which means that the WS0 interrupt will fire when the timeout expires the first time. However, you don't reboot the system during this interrupt. The "panic" will cause the system to halt, but not reboot. Instead, it will just sit there.
the "panic" is not just halt the system, please check the code : (1)It can trigger Kdump (not just print the panic message), if you enable this in the config. that can help server administrator to figure out why the system goes wrong. (2)panic also can trigger a reboot, if you set up "panic timeout".
Obviously, it won't just sit there, it can help user figure out the problem.
At the beginning, I would like to make the first signal more useful, but for simplifying the first version of driver , I decide to use panic(). but if there is better "alerts" for a ARM server , I will go on maintaining this driver to make WS0 more useful.
You're waiting for the WS1 timeout for the system to reboot, but this is not a clean reboot, and it occurs at 2*timeout seconds.
That's why I like my driver better. It doesn't have any of this pretimeout stuff, and when the timeout expires during the WS0 interrupt, it calls emergency_restart() which reboots the system properly. The WS1 hard reset is used as a "backup" reset in case emergency_restart() fails.
OK, If you think so, I hope you can read the SBSA spec more carefully For the watchdog signal (WS0/WS1), SBSA say: "The initial signal is typically wired to an interrupt and alerts the system. The system can attempt to take corrective action that includes refreshing the watchdog within the second watch period. If the refresh is successful the system returns to the previous normal operation.
From here, you can see, even a panic is not good enough. we even can
refreshing the watchdog.
But for simplifying the driver, I think, at least, panic() can help user to backup system context, it is very helpful for a server administrator. Because server should be very stable and important , if its software goes wrong, we must figure out the problem, we can not let it happen again.
but in WS0 interrupt routine , just simply restart , it is not a server watchdog should do.
If it fails then the second watch period expires and a second signal is generated. The signal is fed to a higher agent as an interrupt or reset for it to take executive action."
So WS0 is a warning, but not a reset. the WS1 maybe a reset, or a interrupt to higher agent.
That is different from a normal watchdog use before. the two stage of WS are not just for reset , at least the first one is definitely not a reset. and the second one is not a backup.
If you make SBSA watchdog work like a normal watchdog,: (1)why we need a new driver and new device? you can just use SP805 in the system. (2) why we need a two stages? ( if the second hardware reset signal can work more reliably , why use emergency_restart() which is a software reset, does it clean the system and do some useful backup or sync? ) the only useful thing done by emergency_restart() is kmsg_dump(KMSG_DUMP_EMERG);) (3)why the first WS is connect to a interrupt, but not a reset signal(I believe the direct reset signal is far more reliable than a interrupt to trigger a software reset )
And because of WS0 is a warning, so I decide to use a existing watchdog concept "pretimeout":
Pretimeouts:
Some watchdog timers can be set to have a trigger go off before the actual time they will reset the system. This can be done with an NMI, interrupt, or other mechanism. This allows Linux to record useful information (like panic information and kernel coredumps) before it resets.
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
-- Best regards,
Fu Wei Software Engineer Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch Ph: +86 21 61221326(direct) Ph: +86 186 2020 4684 (mobile) Room 1512, Regus One Corporate Avenue,Level 15, One Corporate Avenue,222 Hubin Road,Huangpu District, Shanghai,China 200021
Fu Wei wrote:
Hi Timur,
On 21 May 2015 at 23:42, Timur Tabi timur@codeaurora.org wrote:
On 05/21/2015 03:32 AM, fu.wei@linaro.org wrote:
+static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{
struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);u64 wcv;wcv = arch_counter_get_cntvct() +(u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;sbsa_gwdt_set_wcv(wdd, wcv);+}
...
+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)+{
wdd->timeout = timeout;return 0;+}
...
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;struct watchdog_device *wdd = &gwdt->wdd;u32 status;status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);if (status & SBSA_GWDT_WCS_WS0)panic("SBSA Watchdog pre-timeout");return IRQ_HANDLED;+}
There's one thing I don't understand about your driver. The 'timeout' value from the kernel is supposed to to be the number of seconds until the system reboots. You are programming the WCV with that value, which means that the WS0 interrupt will fire when the timeout expires the first time. However, you don't reboot the system during this interrupt. The "panic" will cause the system to halt, but not reboot. Instead, it will just sit there.
the "panic" is not just halt the system, please check the code : (1)It can trigger Kdump (not just print the panic message), if you enable this in the config. that can help server administrator to figure out why the system goes wrong. (2)panic also can trigger a reboot, if you set up "panic timeout".
Obviously, it won't just sit there, it can help user figure out the problem.
At the beginning, I would like to make the first signal more useful, but for simplifying the first version of driver , I decide to use panic(). but if there is better "alerts" for a ARM server , I will go on maintaining this driver to make WS0 more useful.
I use emergency_restart(), because the watchdog-api.txt documentation says this:
"If userspace fails (RAM error, kernel bug, whatever), the notifications cease to occur, and the hardware watchdog will reset the system (causing a reboot) after the timeout occurs."
Maybe I'm reading this too literally, but to me this means that when the timeout expires, the system has to reset immediately.
However, maybe panic() is better, since it can do the same thing and more.
So WS0 is a warning, but not a reset. the WS1 maybe a reset, or a interrupt to higher agent.
The watchdog documentation says that the system should reset when the timeout occurs. Therefore, WCV needs to be programming for one-half the timeout value, so that WS1 can occur when the watchdog expires. If the application says, "set timeout to 10 second", then the system has to reboot after 10 second (if the watchdog is pinged).
If the user does not specify a pre-timeout, and if he sets the watchdog to 10 seconds, then WC1 will occur in 20 seconds. You will still call panic() in 10 seconds (during WS0 interrupt), however, so I don't understand how pre-timeout is supposed to work.
That's why I'm confused. I can't tell if you're programming WCV correctly.
On 05/23/2015 12:40 PM, Timur Tabi wrote: [ ... ]
I use emergency_restart(), because the watchdog-api.txt documentation says this:
"If userspace fails (RAM error, kernel bug, whatever), the notifications cease to occur, and the hardware watchdog will reset the system (causing a reboot) after the timeout occurs."
Maybe I'm reading this too literally, but to me this means that when the timeout expires, the system has to reset immediately.
However, maybe panic() is better, since it can do the same thing and more.
I have a specific requirement at work to have watchdog expiration (not this watchdog, this is different HW) result in a panic, specifically to enable crashdump support and thus post-mortem analysis.
I had not thought about this use case myself, and I had always wondered why watchdog driver implementers would choose to call panic() after an interrupt or NMI. But we live and learn, so now I finally understand.
In the pretimeout/timeout world, the pretimeout would (typically) result in a panic, and the timeout would result in a reset. So one would set the timer register to 10s for 10s pretimeout and 20s timeout.
However, the pretimeout concept assumes that there are two timers which can be set independently. As you had pointed out earlier, and as the specification seems to confirm, that is not the case here. As such, I don't really understand why and how the pretimeout / timeout concept would add any value here and not just make things more complicated than necessary. Maybe I am just missing something.
Thanks, Guenter
Guenter Roeck wrote:
However, the pretimeout concept assumes that there are two timers which can be set independently. As you had pointed out earlier, and as the specification seems to confirm, that is not the case here. As such, I don't really understand why and how the pretimeout / timeout concept would add any value here and not just make things more complicated than necessary. Maybe I am just missing something.
It might be possible to load a new value into the WOR register after the WS0 interrupt occurs. That is, in the interrupt handler, we can do something like this:
if (status & SBSA_GWDT_WCS_WS0) // write new WOR value, // then ping watchdog so that it's loaded
I'm not convinced that it's worth it, however. It would require interrupts to still be working when WS0 times out, which somewhat defeats the purpose of a watchdog.
On 05/23/2015 01:27 PM, Timur Tabi wrote:
Guenter Roeck wrote:
However, the pretimeout concept assumes that there are two timers which can be set independently. As you had pointed out earlier, and as the specification seems to confirm, that is not the case here. As such, I don't really understand why and how the pretimeout / timeout concept would add any value here and not just make things more complicated than necessary. Maybe I am just missing something.
It might be possible to load a new value into the WOR register after the WS0 interrupt occurs. That is, in the interrupt handler, we can do something like this:
if (status & SBSA_GWDT_WCS_WS0) // write new WOR value, // then ping watchdog so that it's loadedI'm not convinced that it's worth it, however. It would require interrupts to still be working when WS0 times out, which somewhat defeats the purpose of a watchdog.
If I understand the specification correctly, reloading the register would result in another WS0, not in WS1. That isn't really what we would want to happen.
Reloading the register would normally be done in the crashdump kernel, if it is loaded, to give it time to actually take the crashdump. But that is post-restart, not pre-restart.
Thanks, Guenter
Hi Guenter,
On 24 May 2015 at 04:44, Guenter Roeck linux@roeck-us.net wrote:
On 05/23/2015 01:27 PM, Timur Tabi wrote:
Guenter Roeck wrote:
However, the pretimeout concept assumes that there are two timers which can be set independently. As you had pointed out earlier, and as the specification seems to confirm, that is not the case here. As such, I don't really understand why and how the pretimeout / timeout concept would add any value here and not just make things more complicated than necessary. Maybe I am just missing something.
It might be possible to load a new value into the WOR register after the WS0 interrupt occurs. That is, in the interrupt handler, we can do something like this:
if (status & SBSA_GWDT_WCS_WS0) // write new WOR value, // then ping watchdog so that it's loadedI'm not convinced that it's worth it, however. It would require interrupts to still be working when WS0 times out, which somewhat defeats the purpose of a watchdog.
If I understand the specification correctly, reloading the register would result in another WS0, not in WS1. That isn't really what we would want to happen.
Yes, you are 100% correct. In SBSA: --------------- An explicit watchdog refresh occurs when one of a number of different events occur: (1) The Watchdog Refresh Register is written. (2) The Watchdog Offset Register is written. (3) The Watchdog Control and Status register is written. In the case of an explicit refresh the Watchdog Signals are cleared. --------------- So, for the second timeout, we can only use WOR.(but maybe we can write WCV in the WS0 routine to make the second timeout longer, if we really need a long time for panic) If we write WOR , that will clean WCS, then the system go back to first timeout stage.
But for the first timeout, we can use WOR (that means the two timeout stages will be the same, in another world, WS0==WS1*2 ) or we can write WCV directly.
And writing WCV will not trigger an explicit watchdog refresh, that is what I am doing to set up pretimeout.
Reloading the register would normally be done in the crashdump kernel, if it is loaded, to give it time to actually take the crashdump. But that is post-restart, not pre-restart.
Thanks, Guenter
Hi Guenter,
On 24 May 2015 at 04:01, Guenter Roeck linux@roeck-us.net wrote:
On 05/23/2015 12:40 PM, Timur Tabi wrote: [ ... ]
I use emergency_restart(), because the watchdog-api.txt documentation says this:
"If userspace fails (RAM error, kernel bug, whatever), the notifications cease to occur, and the hardware watchdog will reset the system (causing a reboot) after the timeout occurs."
Maybe I'm reading this too literally, but to me this means that when the timeout expires, the system has to reset immediately.
However, maybe panic() is better, since it can do the same thing and more.
I have a specific requirement at work to have watchdog expiration (not this watchdog, this is different HW) result in a panic, specifically to enable crashdump support and thus post-mortem analysis.
I had not thought about this use case myself, and I had always wondered why watchdog driver implementers would choose to call panic() after an interrupt or NMI. But we live and learn, so now I finally understand.
In the pretimeout/timeout world, the pretimeout would (typically) result in a panic, and the timeout would result in a reset. So one would set the timer register to 10s for 10s pretimeout and 20s timeout.
However, the pretimeout concept assumes that there are two timers which can be set independently. As you had pointed out earlier, and as the specification seems to confirm, that is not the case here.
Sorry, in Documentation/watchdog/watchdog-api.txt, I can not get the info about " the pretimeout concept assumes that there are two timers which can be set independently." Could you kindly point out where is the assumption.
I thinks in kernel documentation, that meams "one watchdog has two timeout stages", maybe I miss something. Could you help me out?
As such, I don't really understand why and how the pretimeout / timeout concept would add any value here and not just make things more complicated than necessary. Maybe I am just missing something.
If pretimeout concept assumes that there are two timers, I misunderstand the "pretimeout", then I will delete the pretimeout immediately.
Thanks, Guenter
On 05/24/2015 03:15 AM, Fu Wei wrote:
Hi Guenter,
On 24 May 2015 at 04:01, Guenter Roeck linux@roeck-us.net wrote:
On 05/23/2015 12:40 PM, Timur Tabi wrote: [ ... ]
I use emergency_restart(), because the watchdog-api.txt documentation says this:
"If userspace fails (RAM error, kernel bug, whatever), the notifications cease to occur, and the hardware watchdog will reset the system (causing a reboot) after the timeout occurs."
Maybe I'm reading this too literally, but to me this means that when the timeout expires, the system has to reset immediately.
However, maybe panic() is better, since it can do the same thing and more.
I have a specific requirement at work to have watchdog expiration (not this watchdog, this is different HW) result in a panic, specifically to enable crashdump support and thus post-mortem analysis.
I had not thought about this use case myself, and I had always wondered why watchdog driver implementers would choose to call panic() after an interrupt or NMI. But we live and learn, so now I finally understand.
In the pretimeout/timeout world, the pretimeout would (typically) result in a panic, and the timeout would result in a reset. So one would set the timer register to 10s for 10s pretimeout and 20s timeout.
However, the pretimeout concept assumes that there are two timers which can be set independently. As you had pointed out earlier, and as the specification seems to confirm, that is not the case here.
Sorry, in Documentation/watchdog/watchdog-api.txt, I can not get the info about " the pretimeout concept assumes that there are two timers which can be set independently." Could you kindly point out where is the assumption.
I thinks in kernel documentation, that meams "one watchdog has two timeout stages", maybe I miss something. Could you help me out?
My apologies. Terminology problem; see below.
Note that the pretimeout, as documented, is a difference to the real timeout, not an absolute time (which I had not realized before).
"Note that the pretimeout is the number of seconds before the time when the timeout will go off. It is not the number of seconds until the pretimeout. So, for instance, if you set the timeout to 60 seconds and the pretimeout to 10 seconds, the pretimeout will go off in 50 seconds. Setting a pretimeout to zero disables it."
As such, I don't really understand why and how the pretimeout / timeout concept would add any value here and not just make things more complicated than necessary. Maybe I am just missing something.
If pretimeout concept assumes that there are two timers, I misunderstand the "pretimeout", then I will delete the pretimeout immediately.
I think I used the wrong term. I should have said something like "two distinct timeout values".
Guenter
Hi Guenter,
Thanks for your feedback
On 24 May 2015 at 22:15, Guenter Roeck linux@roeck-us.net wrote:
On 05/24/2015 03:15 AM, Fu Wei wrote:
Hi Guenter,
On 24 May 2015 at 04:01, Guenter Roeck linux@roeck-us.net wrote:
On 05/23/2015 12:40 PM, Timur Tabi wrote: [ ... ]
I use emergency_restart(), because the watchdog-api.txt documentation says this:
"If userspace fails (RAM error, kernel bug, whatever), the notifications cease to occur, and the hardware watchdog will reset the system (causing a reboot) after the timeout occurs."
Maybe I'm reading this too literally, but to me this means that when the timeout expires, the system has to reset immediately.
However, maybe panic() is better, since it can do the same thing and more.
I have a specific requirement at work to have watchdog expiration (not this watchdog, this is different HW) result in a panic, specifically to enable crashdump support and thus post-mortem analysis.
I had not thought about this use case myself, and I had always wondered why watchdog driver implementers would choose to call panic() after an interrupt or NMI. But we live and learn, so now I finally understand.
In the pretimeout/timeout world, the pretimeout would (typically) result in a panic, and the timeout would result in a reset. So one would set the timer register to 10s for 10s pretimeout and 20s timeout.
However, the pretimeout concept assumes that there are two timers which can be set independently. As you had pointed out earlier, and as the specification seems to confirm, that is not the case here.
Sorry, in Documentation/watchdog/watchdog-api.txt, I can not get the info about " the pretimeout concept assumes that there are two timers which can be set independently." Could you kindly point out where is the assumption.
I thinks in kernel documentation, that meams "one watchdog has two timeout stages", maybe I miss something. Could you help me out?
My apologies. Terminology problem; see below.
Note that the pretimeout, as documented, is a difference to the real timeout, not an absolute time (which I had not realized before).
"Note that the pretimeout is the number of seconds before the time when the timeout will go off. It is not the number of seconds until the pretimeout. So, for instance, if you set the timeout to 60 seconds and the pretimeout to 10 seconds, the pretimeout will go off in 50 seconds. Setting a pretimeout to zero disables it."
yes , this patchset is designed for this pretimeout concept
As such, I don't really understand why and how the pretimeout / timeout concept would add any value here and not just make things more complicated than necessary. Maybe I am just missing something.
If pretimeout concept assumes that there are two timers, I misunderstand the "pretimeout", then I will delete the pretimeout immediately.
I think I used the wrong term. I should have said something like "two distinct timeout values".
Actually, I have added my thought at the head of sbsa_gwdt.c as a comment :
* * Note: This SBSA Generic watchdog driver is compatible with * the pretimeout concept of Linux kernel. * The timeout and pretimeout are set by the different REGs. * The first watch period is set by writing WCV directly, * that can support more than 10s timeout at the maximum * system counter frequency. * The second watch period is set by WOR(32bit) which will be loaded * automatically by hardware, when WS0 is triggered. * This gives a maximum watch period of around 10s at the maximum * system counter frequency. * The System Counter shall run at maximum of 400MHz. * More details: DEN0029B - Server Base System Architecture (SBSA) * * Kernel/API: P---------| pretimeout * |-------------------------------T timeout * SBSA GWDT: P--WOR---WS1 pretimeout * |-------WCV----------WS0~~~~~~~~T timeout */
Guenter
On 05/24/2015 08:50 AM, Fu Wei wrote: [ ...]
Actually, I have added my thought at the head of sbsa_gwdt.c as a comment :
- Note: This SBSA Generic watchdog driver is compatible with
the pretimeout concept of Linux kernel.The timeout and pretimeout are set by the different REGs.The first watch period is set by writing WCV directly,that can support more than 10s timeout at the maximumsystem counter frequency.The second watch period is set by WOR(32bit) which will be loadedautomatically by hardware, when WS0 is triggered.This gives a maximum watch period of around 10s at the maximumsystem counter frequency.The System Counter shall run at maximum of 400MHz.More details: DEN0029B - Server Base System Architecture (SBSA)- Kernel/API: P---------| pretimeout
|-------------------------------T timeout- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout*/
Yes, but do we actually _know_ that it works that way, ie that WCV drives WS0 and that WOR drives WS1 ? Unless I am missing something, the specification doesn't say that, and it would have been a really easy statement to make if that was the intent.
My concern here is that the above behavior is not spelled out in the document, meaning it is up to interpretation by the hardware engineer implementing it, to the point where it appears that not even two software engineers can agree how it is supposed to work. Which is a really bad starting point :-(.
Thanks, Guenter
Hi Guenter,
On 25 May 2015 at 00:23, Guenter Roeck linux@roeck-us.net wrote:
On 05/24/2015 08:50 AM, Fu Wei wrote: [ ...]
Actually, I have added my thought at the head of sbsa_gwdt.c as a comment :
- Note: This SBSA Generic watchdog driver is compatible with
the pretimeout concept of Linux kernel.The timeout and pretimeout are set by the different REGs.The first watch period is set by writing WCV directly,that can support more than 10s timeout at the maximumsystem counter frequency.The second watch period is set by WOR(32bit) which will beloaded
automatically by hardware, when WS0 is triggered.This gives a maximum watch period of around 10s at the maximumsystem counter frequency.The System Counter shall run at maximum of 400MHz.More details: DEN0029B - Server Base System Architecture (SBSA)- Kernel/API: P---------| pretimeout
|-------------------------------T timeout- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout*/
Yes, but do we actually _know_ that it works that way, ie that WCV drives WS0 and that WOR drives WS1 ? Unless I am missing something, the specification doesn't say that, and it would have been a really easy statement to make if that was the intent.
yes, Suravee has tested it on Seattle B0 Soc, that works. But hope Suravee can provide more info about test, I will ping him later.
According to SBSA, that WCV and WOR can both drive WS1 and WS0
the timeout and pretimeout in my patchset have been tested on Seattle B0 and Foundation model.
My concern here is that the above behavior is not spelled out in the document, meaning it is up to interpretation by the hardware engineer implementing it, to the point where it appears that not even two software engineers can agree how it is supposed to work. Which is a really bad starting point :-(.
Is that a real hardware teat can prove it works?
or actually, SBSA say that it should work: ----------------- Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register. ----------------- offset register == WOR compare value register == WCV
Thanks, Guenter
-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/24/2015 09:47 AM, Fu Wei wrote:
Hi Guenter,
On 25 May 2015 at 00:23, Guenter Roeck linux@roeck-us.net wrote:
On 05/24/2015 08:50 AM, Fu Wei wrote: [ ...]
Actually, I have added my thought at the head of sbsa_gwdt.c as a comment :
*
- Note: This SBSA Generic watchdog driver is compatible with
the pretimeout concept of Linux kernel.The timeout and pretimeout are set by the different REGs.The first watch period is set by writing WCV directly,that can support more than 10s timeout at the maximumsystem counter frequency.The second watch period is set by WOR(32bit) which will beloaded
automatically by hardware, when WS0 is triggered.This gives a maximum watch period of around 10s at the maximumsystem counter frequency.The System Counter shall run at maximum of 400MHz.More details: DEN0029B - Server Base System Architecture (SBSA)- Kernel/API: P---------| pretimeout
|-------------------------------T timeout- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout*/
Yes, but do we actually _know_ that it works that way, ie that WCV drives WS0 and that WOR drives WS1 ? Unless I am missing something, the specification doesn't say that, and it would have been a really easy statement to make if that was the intent.
yes, Suravee has tested it on Seattle B0 Soc, that works. But hope Suravee can provide more info about test, I will ping him later.
According to SBSA, that WCV and WOR can both drive WS1 and WS0
the timeout and pretimeout in my patchset have been tested on Seattle B0 and Foundation model.
My concern here is that the above behavior is not spelled out in the document, meaning it is up to interpretation by the hardware engineer implementing it, to the point where it appears that not even two software engineers can agree how it is supposed to work. Which is a really bad starting point :-(.
Is that a real hardware teat can prove it works?
or actually, SBSA say that it should work:
Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register.
offset register == WOR compare value register == WCV
Where does it say that WCV shall be associated with WS0 and that WOR shall be associated with WS1 ? Guess I am missing that part.
Thanks, Guenter
Hi Guenter,
On 25 May 2015 at 00:58, Guenter Roeck linux@roeck-us.net wrote:
On 05/24/2015 09:47 AM, Fu Wei wrote:
Hi Guenter,
On 25 May 2015 at 00:23, Guenter Roeck linux@roeck-us.net wrote:
On 05/24/2015 08:50 AM, Fu Wei wrote: [ ...]
Actually, I have added my thought at the head of sbsa_gwdt.c as a comment :
*
- Note: This SBSA Generic watchdog driver is compatible with
the pretimeout concept of Linux kernel.The timeout and pretimeout are set by the different REGs.The first watch period is set by writing WCV directly,that can support more than 10s timeout at the maximumsystem counter frequency.The second watch period is set by WOR(32bit) which will beloaded
automatically by hardware, when WS0 is triggered.This gives a maximum watch period of around 10s at themaximum
system counter frequency.The System Counter shall run at maximum of 400MHz.More details: DEN0029B - Server Base System Architecture(SBSA)
- Kernel/API: P---------| pretimeout
|-------------------------------T timeout- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout*/
Yes, but do we actually _know_ that it works that way, ie that WCV drives WS0 and that WOR drives WS1 ? Unless I am missing something, the specification doesn't say that, and it would have been a really easy statement to make if that was the intent.
yes, Suravee has tested it on Seattle B0 Soc, that works. But hope Suravee can provide more info about test, I will ping him later.
According to SBSA, that WCV and WOR can both drive WS1 and WS0
the timeout and pretimeout in my patchset have been tested on Seattle B0 and Foundation model.
My concern here is that the above behavior is not spelled out in the document, meaning it is up to interpretation by the hardware engineer implementing it, to the point where it appears that not even two software engineers can agree how it is supposed to work. Which is a really bad starting point :-(.
Is that a real hardware teat can prove it works?
or actually, SBSA say that it should work:
Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register.
offset register == WOR compare value register == WCV
Where does it say that WCV shall be associated with WS0 and that WOR shall be associated with WS1 ? Guess I am missing that part.
no, it doesn't say WCV shall be associated with WS0, that is my driver doing.
but WCV can control both WS1/WS0. WOR is just a auto-reload value.
I think maybe you can read SBSA 2.3 Page 23, The pseudocode can explain everything. :-)
Great thanks for your time
Thanks, Guenter
-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fu Wei wrote:
If pretimeout concept assumes that there are two timers, I misunderstand the "pretimeout", then I will delete the pretimeout immediately.
In my opinion, calling panic() on a pre-timeout is not useful, because that's really just a normal timeout. If there were a way to "warn" user space that a timeout is about to occur, without a panic or reset, then that might be useful. But as far as I can see, all you're doing is redefining the word "timeout".
Hi Timur,
I have said this before:
in the first timeout, just panic() maybe not enough, in [RFC] version of my patchset, I offer some option as "preaction" to use, but for simplifying the first version of driver, I have deleted them. but at least, panic() is far more useful than a simple reset. at least, it can provide the context of the crashed system to admin.
If you want to warn user space, that will make driver more complicated, I don't think that is a good choose for a first version. but we can find a way to improve this later
On 24 May 2015 at 23:02, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
If pretimeout concept assumes that there are two timers, I misunderstand the "pretimeout", then I will delete the pretimeout immediately.
In my opinion, calling panic() on a pre-timeout is not useful, because that's really just a normal timeout. If there were a way to "warn" user space that a timeout is about to occur, without a panic or reset, then that might be useful. But as far as I can see, all you're doing is redefining the word "timeout".
-- 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.
Fu Wei wrote:
in the first timeout, just panic() maybe not enough, in [RFC] version of my patchset, I offer some option as "preaction" to use, but for simplifying the first version of driver, I have deleted them. but at least, panic() is far more useful than a simple reset. at least, it can provide the context of the crashed system to admin.
My point is that there is very little difference between
1) calling panic() on pre-timeout 2) calling panic() on timeout
In both cases, the system will panic. The watchdog API says that the system should reset when a timeout occurs, so you cannot call panic() before the timeout expires.
If you want to warn user space, that will make driver more complicated, I don't think that is a good choose for a first version. but we can find a way to improve this later
In my opinion, this "first version" is not useful. I would like to see a pre-timeout feature that does not panic or reset when a pre-timeout occurs.
On 05/24/2015 09:13 AM, Timur Tabi wrote:
Fu Wei wrote:
in the first timeout, just panic() maybe not enough, in [RFC] version of my patchset, I offer some option as "preaction" to use, but for simplifying the first version of driver, I have deleted them. but at least, panic() is far more useful than a simple reset. at least, it can provide the context of the crashed system to admin.
My point is that there is very little difference between
- calling panic() on pre-timeout
- calling panic() on timeout
The assumption would be that the second timeout doesn't cause a panic but a system reset.
In both cases, the system will panic. The watchdog API says that the system should reset when a timeout occurs, so you cannot call panic() before the timeout expires.
If you want to warn user space, that will make driver more complicated, I don't think that is a good choose for a first version. but we can find a way to improve this later
In my opinion, this "first version" is not useful. I would like to see a pre-timeout feature that does not panic or reset when a pre-timeout occurs.
The current watchdog API suggests that the pretimeout "allows Linux to record useful information (like panic information and kernel coredumps) before it resets". The call to panic() would be the means to make this happen.
Are you suggesting to change this definition ? What should it do instead in your opinion ?
Thanks, Guenter
Hi Guenter
On 25 May 2015 at 00:29, Guenter Roeck linux@roeck-us.net wrote:
On 05/24/2015 09:13 AM, Timur Tabi wrote:
Fu Wei wrote:
in the first timeout, just panic() maybe not enough, in [RFC] version of my patchset, I offer some option as "preaction" to use, but for simplifying the first version of driver, I have deleted them. but at least, panic() is far more useful than a simple reset. at least, it can provide the context of the crashed system to admin.
My point is that there is very little difference between
- calling panic() on pre-timeout
- calling panic() on timeout
The assumption would be that the second timeout doesn't cause a panic but a system reset.
In both cases, the system will panic. The watchdog API says that the system should reset when a timeout occurs, so you cannot call panic() before the timeout expires.
If you want to warn user space, that will make driver more complicated, I don't think that is a good choose for a first version. but we can find a way to improve this later
In my opinion, this "first version" is not useful. I would like to see a pre-timeout feature that does not panic or reset when a pre-timeout occurs.
The current watchdog API suggests that the pretimeout "allows Linux to record useful information (like panic information and kernel coredumps) before it resets". The call to panic() would be the means to make this happen.
Yes, that is what I mean. Great thanks for your explanation. :-)
Are you suggesting to change this definition ? What should it do instead in your opinion ?
Thanks, Guenter
Guenter Roeck wrote:
The current watchdog API suggests that the pretimeout "allows Linux to record useful information (like panic information and kernel coredumps) before it resets". The call to panic() would be the means to make this happen.
Now that I think about it, that does make sense.
However, if a pre-timeout is not set, then the driver should never call panic(). That means that the interrupt handler should only be registered if sbsa_gwdt_set_pretimeout() is called.
On 05/24/2015 09:44 AM, Timur Tabi wrote:
Guenter Roeck wrote:
The current watchdog API suggests that the pretimeout "allows Linux to record useful information (like panic information and kernel coredumps) before it resets". The call to panic() would be the means to make this happen.
Now that I think about it, that does make sense.
However, if a pre-timeout is not set, then the driver should never call panic(). That means that the interrupt handler should only be registered if sbsa_gwdt_set_pretimeout() is called.
This is a matter of opinion. Some watchdogs have two (or even more) levels of timeouts without explicit pretimeout. The iTCO watchdog in Intel systems is an example, but there are several others.
For such systems, there may be an initial interrupt followed by a hard reset a little later. In such cases it does make sense to wire up the interrupt and have it call panic(). If the system gets stuck, the second timeout will reset it.
Guenter
Hi Timur
On 25 May 2015 at 00:44, Timur Tabi timur@codeaurora.org wrote:
Guenter Roeck wrote:
The current watchdog API suggests that the pretimeout "allows Linux to record useful information (like panic information and kernel coredumps) before it resets". The call to panic() would be the means to make this happen.
Now that I think about it, that does make sense.
However, if a pre-timeout is not set, then the driver should never call panic(). That means that the interrupt handler should only be registered if sbsa_gwdt_set_pretimeout() is called.
I don't know why you want to do this tricky way. you can always register the interrupt handler, if pre-timeout is 0, system will just trigger WS1 right after WS0
-- 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.
Fu Wei wrote:
I don't know why you want to do this tricky way. you can always register the interrupt handler, if pre-timeout is 0, system will just trigger WS1 right after WS0
But that only works if the pre-timeout and timeout can be programmed to separate values. And as Guenter says, the SBSA may not guarantee that.
Hi Timur,
On 25 May 2015 at 01:19, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
I don't know why you want to do this tricky way. you can always register the interrupt handler, if pre-timeout is 0, system will just trigger WS1 right after WS0
But that only works if the pre-timeout and timeout can be programmed to separate values. And as Guenter says, the SBSA may not guarantee that.
In my driver patch, the first stage, I am using WCV, for the second stage I am using WOR, that have been tested on real hardware.
would you please read the pseudocode in Page 23 of SBSA 2.3
-- 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 05/24/2015 10:19 AM, Timur Tabi wrote:
Fu Wei wrote:
I don't know why you want to do this tricky way. you can always register the interrupt handler, if pre-timeout is 0, system will just trigger WS1 right after WS0
But that only works if the pre-timeout and timeout can be programmed to separate values. And as Guenter says, the SBSA may not guarantee that.
The pseudo-code in the specification suggests that if WCV is configured, WS0 = WCV WS1 = WCV + WOR
Assuming that the implementation follows the pseudo-code in the specification, we would have separately programmable values. Since the pretimeout (per ABI) is the difference in seconds to the timeout, and not an absolute value, we would have to program the registers as follows.
WCV = timeout - pretimeout; WOR = pretimeout;
Does this make sense ?
Thanks, Guenter
Guenter Roeck wrote:
The pseudo-code in the specification suggests that if WCV is configured, WS0 = WCV WS1 = WCV + WOR
Assuming that the implementation follows the pseudo-code in the specification, we would have separately programmable values. Since the pretimeout (per ABI) is the difference in seconds to the timeout, and not an absolute value, we would have to program the registers as follows.
WCV = timeout - pretimeout; WOR = pretimeout;Does this make sense ?
Yes, thank you. I will test this on my hardware.
Hi Guenter,
On 25 May 2015 at 01:47, Timur Tabi timur@codeaurora.org wrote:
Guenter Roeck wrote:
The pseudo-code in the specification suggests that if WCV is configured, WS0 = WCV WS1 = WCV + WOR
Assuming that the implementation follows the pseudo-code in the specification, we would have separately programmable values. Since the pretimeout (per ABI) is the difference in seconds to the timeout, and not an absolute value, we would have to program the registers as follows.
WCV = timeout - pretimeout; WOR = pretimeout;Does this make sense ?
And actually all the patchset I sent to upstream are all follow this design, and have been test on Foundation model and Seattle B0
Yes, thank you. I will test this on my hardware.
-- 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.
Hi, Guenter,
On 25 May 2015 at 01:32, Guenter Roeck linux@roeck-us.net wrote:
On 05/24/2015 10:19 AM, Timur Tabi wrote:
Fu Wei wrote:
I don't know why you want to do this tricky way. you can always register the interrupt handler, if pre-timeout is 0, system will just trigger WS1 right after WS0
But that only works if the pre-timeout and timeout can be programmed to separate values. And as Guenter says, the SBSA may not guarantee that.
The pseudo-code in the specification suggests that if WCV is configured, WS0 = WCV WS1 = WCV + WOR
Assuming that the implementation follows the pseudo-code in the specification, we would have separately programmable values. Since the pretimeout (per ABI) is the difference in seconds to the timeout, and not an absolute value, we would have to program the registers as follows.
WCV = timeout - pretimeout; WOR = pretimeout;
yes, this patchset is doing this way.
Does this make sense ?
Thanks, Guenter
Hi Timur,
Sorry, how do you trigger a panic if WS1 ties to reset ?? don't know what is your question
could you make it simple:
1) calling panic() on pre-timeout (WS0????) 2) calling panic() on timeout (WS1????)
in "first version" , I just print the timeleft for WS1 or panic when WS0 occurs.
On 25 May 2015 at 00:13, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
in the first timeout, just panic() maybe not enough, in [RFC] version of my patchset, I offer some option as "preaction" to use, but for simplifying the first version of driver, I have deleted them. but at least, panic() is far more useful than a simple reset. at least, it can provide the context of the crashed system to admin.
My point is that there is very little difference between
- calling panic() on pre-timeout
- calling panic() on timeout
In both cases, the system will panic. The watchdog API says that the system should reset when a timeout occurs, so you cannot call panic() before the timeout expires.
If you want to warn user space, that will make driver more complicated, I don't think that is a good choose for a first version. but we can find a way to improve this later
In my opinion, this "first version" is not useful. I would like to see a pre-timeout feature that does not panic or reset when a pre-timeout occurs.
-- 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 2015年05月21日 16:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This driver bases on linux kernel watchdog framework, and use "pretimeout" in the framework. It supports getting timeout and pretimeout from parameter and FDT at the driver init stage. In first timeout(WS0), the interrupt routine run panic to save system context.
Signed-off-by: Fu Wei fu.wei@linaro.org
drivers/watchdog/Kconfig | 12 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TEST
SBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, and why we depends on COMPILE_TEST?
- depends on ARM_ARCH_TIMER
- select WATCHDOG_CORE
- help
ARM SBSA Generic Watchdog timer. This has two Watchdog Signals(WS0/WS1), will trigger a warning interrupt(do panic) at the firsttimeout(WS0); will reboot your system when the second timeout(WS1)is reached.More details: DEN0029B - Server Base System Architecture (SBSA)- config AT91RM9200_WATCHDOG tristate "AT91RM9200 watchdog" depends on SOC_AT91RM9200 && MFD_SYSCON
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..471f1b7c 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_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o diff --git a/drivers/watchdog/sbsa_gwdt.c b/drivers/watchdog/sbsa_gwdt.c new file mode 100644 index 0000000..4ebe7c3 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,476 @@ +/*
- 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>
- 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.
- Note: This SBSA Generic watchdog driver is compatible with
the pretimeout concept of Linux kernel.
The timeout and pretimeout are set by the different REGs.
The first watch period is set by writing WCV directly,
that can support more than 10s timeout at the maximum
system counter frequency.
The second watch period is set by WOR(32bit) which will be loaded
automatically by hardware, when WS0 is triggered.
This gives a maximum watch period of around 10s at the maximum
system counter frequency.
The System Counter shall run at maximum of 400MHz.
More details: DEN0029B - Server Base System Architecture (SBSA)
- Kernel/API: P---------| pretimeout
|-------------------------------T timeout
- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout- */
+#include <asm/arch_timer.h>
please put it under #include <linux/*.h>, this is the convention of order head files.
+#include <linux/acpi.h>
Will we add any acpi realted code in this patch? if not, please remove this head file.
+#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>
+/* 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_LO 0x010 +#define SBSA_GWDT_WCV_HI 0x014
+/* 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 10 /* seconds, the 1st + 2nd watch periods*/ +#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/
+static unsigned int timeout; +module_param(timeout, uint, 0); +MODULE_PARM_DESC(timeout,
"Watchdog timeout in seconds. (>=0, default="__MODULE_STRING(DEFAULT_TIMEOUT) ")");+static unsigned int max_timeout = UINT_MAX; +module_param(max_timeout, uint, 0); +MODULE_PARM_DESC(max_timeout,
"Watchdog max timeout in seconds. (>=0, default="__MODULE_STRING(UINT_MAX) ")");+static unsigned int pretimeout; +module_param(pretimeout, uint, 0); +MODULE_PARM_DESC(pretimeout,
"Watchdog pretimeout in seconds. (>=0, default="__MODULE_STRING(DEFAULT_PRETIMEOUT) ")");+static unsigned int max_pretimeout = U32_MAX; +module_param(max_pretimeout, uint, 0); +MODULE_PARM_DESC(max_pretimeout,
"Watchdog max pretimeout in seconds. (>=0, default="__MODULE_STRING(U32_MAX) ")");+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) ")");+/*
- Architected system timer support.
- */
+static void sbsa_gwdt_cf_write(unsigned int reg, u32 val,
struct watchdog_device *wdd)+{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- writel_relaxed(val, gwdt->control_base + reg);
+}
+static void sbsa_gwdt_rf_write(unsigned int reg, u32 val,
struct watchdog_device *wdd)+{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- writel_relaxed(val, gwdt->refresh_base + reg);
+}
+static u32 sbsa_gwdt_cf_read(unsigned int reg, struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- return readl_relaxed(gwdt->control_base + reg);
+}
+/*
- help founctions for accessing 64bit WCV register
- */
+static u64 sbsa_gwdt_get_wcv(struct watchdog_device *wdd) +{
- u32 wcv_lo, wcv_hi;
- do {
wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd);wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd);- } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd));
- return (((u64)wcv_hi << 32) | wcv_lo);
+}
+static void sbsa_gwdt_set_wcv(struct watchdog_device *wdd, u64 value) +{
- u32 wcv_lo, wcv_hi;
- wcv_lo = value & U32_MAX;
- wcv_hi = (value >> 32) & U32_MAX;
- sbsa_gwdt_cf_write(SBSA_GWDT_WCV_HI, wcv_hi, wdd);
- sbsa_gwdt_cf_write(SBSA_GWDT_WCV_LO, wcv_lo, wdd);
+}
+static void reload_timeout_to_wcv(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 wcv;
- wcv = arch_counter_get_cntvct() +
(u64)(wdd->timeout - wdd->pretimeout) * gwdt->clk;- sbsa_gwdt_set_wcv(wdd, wcv);
+}
+/*
- Use the following function to set the limit of timeout
- after updating pretimeout
- */
+static void sbsa_gwdt_update_limits(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 first_period_max = U64_MAX;
- do_div(first_period_max, gwdt->clk);
- wdd->min_timeout = wdd->pretimeout + 1;
- wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),
wdd->max_timeout);+}
+static int sbsa_gwdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)+{
- wdd->timeout = timeout;
- return 0;
+}
+static int sbsa_gwdt_set_pretimeout(struct watchdog_device *wdd,
unsigned int pretimeout)+{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u32 wor;
- wdd->pretimeout = pretimeout;
- sbsa_gwdt_update_limits(wdd);
- if (!pretimeout)
/* gives sbsa_gwdt_start a chance to setup timeout */wor = gwdt->clk;- else
wor = pretimeout * gwdt->clk;- /* refresh the WOR, that will cause an explicit watchdog refresh */
- sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wor, wdd);
- return 0;
+}
+static unsigned int sbsa_gwdt_get_timeleft(struct watchdog_device *wdd) +{
- struct sbsa_gwdt *gwdt = to_sbsa_gwdt(wdd);
- u64 timeleft = sbsa_gwdt_get_wcv(wdd) - arch_counter_get_cntvct();
- do_div(timeleft, gwdt->clk);
- return timeleft;
+}
+static int sbsa_gwdt_start(struct watchdog_device *wdd) +{
- /* Force refresh */
- sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
- /* writing WCS will cause an explicit watchdog refresh */
- sbsa_gwdt_cf_write(SBSA_GWDT_WCS, SBSA_GWDT_WCS_EN, wdd);
- reload_timeout_to_wcv(wdd);
- return 0;
+}
+static int sbsa_gwdt_stop(struct watchdog_device *wdd) +{
- /* Force refresh */
- sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
- /* writing WCS will cause an explicit watchdog refresh */
- sbsa_gwdt_cf_write(SBSA_GWDT_WCS, 0, wdd);
- return 0;
+}
+static int sbsa_gwdt_keepalive(struct watchdog_device *wdd) +{
- /*
* Writing WRR for an explicit watchdog refresh* You can write anyting(like 0xc0ffee)*/- sbsa_gwdt_rf_write(SBSA_GWDT_WRR, 0xc0ffee, wdd);
- reload_timeout_to_wcv(wdd);
- return 0;
+}
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
- struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
- struct watchdog_device *wdd = &gwdt->wdd;
- u32 status;
- status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
- if (status & SBSA_GWDT_WCS_WS0)
panic("SBSA Watchdog pre-timeout");- return IRQ_HANDLED;
+}
+static struct watchdog_info sbsa_gwdt_info = {
- .identity = "SBSA Generic Watchdog",
- .options = WDIOF_SETTIMEOUT |
WDIOF_KEEPALIVEPING |WDIOF_MAGICCLOSE |WDIOF_PRETIMEOUT |WDIOF_CARDRESET,+};
+static struct watchdog_ops sbsa_gwdt_ops = {
- .owner = THIS_MODULE,
- .start = sbsa_gwdt_start,
- .stop = sbsa_gwdt_stop,
- .ping = sbsa_gwdt_keepalive,
- .set_timeout = sbsa_gwdt_set_timeout,
- .set_pretimeout = sbsa_gwdt_set_pretimeout,
- .get_timeleft = sbsa_gwdt_get_timeleft,
+};
+static int sbsa_gwdt_probe(struct platform_device *pdev) +{
- struct device *dev = &pdev->dev;
- struct sbsa_gwdt *gwdt;
- struct watchdog_device *wdd;
- struct resource *res;
- void *rf_base, *cf_base;
- int irq;
- u32 clk, status;
- int ret = 0;
- u64 first_period_max = U64_MAX;
- /*
* Try to determine the frequency from the arch_timer interface*/- clk = arch_timer_get_rate();
- if (!clk) {
dev_err(dev, "System Counter frequency not available\n");return -EINVAL;- }
- gwdt = devm_kzalloc(dev, sizeof(*gwdt), GFP_KERNEL);
- if (!gwdt)
return -ENOMEM;- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "refresh");
- rf_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(rf_base))
return PTR_ERR(rf_base);- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "control");
- cf_base = devm_ioremap_resource(dev, res);
- if (IS_ERR(rf_base))
return PTR_ERR(rf_base);- irq = platform_get_irq_byname(pdev, "ws0");
- if (irq < 0) {
dev_err(dev, "unable to get ws0 interrupt.\n");return irq;- }
- gwdt->refresh_base = rf_base;
- gwdt->control_base = cf_base;
- gwdt->clk = clk;
- platform_set_drvdata(pdev, gwdt);
- 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);
- status = sbsa_gwdt_cf_read(SBSA_GWDT_WCS, wdd);
- if (status & SBSA_GWDT_WCS_WS1) {
dev_warn(dev, "System reset by WDT(WCS: %x, WCV: %llx)\n",status, sbsa_gwdt_get_wcv(wdd));wdd->bootstatus |= WDIOF_CARDRESET;- }
- /* Check if watchdog is already enabled */
- if (status & SBSA_GWDT_WCS_EN) {
dev_warn(dev, "already enabled! WCS:0x%x\n", status);sbsa_gwdt_keepalive(wdd);- }
- wdd->min_pretimeout = 0;
- wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout);
- wdd->min_timeout = 1;
- do_div(first_period_max, clk);
- wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),
max_timeout);- wdd->pretimeout = DEFAULT_PRETIMEOUT;
- wdd->timeout = DEFAULT_TIMEOUT;
- watchdog_init_timeouts(wdd, pretimeout, timeout,
sbsa_gwdt_update_limits, dev);- /* update pretimeout to WOR */
- sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd);
- ret = devm_request_irq(dev, irq, sbsa_gwdt_interrupt, IRQF_TIMER,
pdev->name, gwdt);- if (ret) {
dev_err(dev, "unable to request IRQ %d\n", irq);return ret;- }
- ret = watchdog_register_device(wdd);
- if (ret)
return ret;- dev_info(dev, "Initialized with %ds timeout, %ds pretimeout @ %uHz\n",
wdd->timeout, wdd->pretimeout, gwdt->clk);- 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);
- int ret = 0;
- if (!nowayout)
ret = sbsa_gwdt_stop(&gwdt->wdd);- watchdog_unregister_device(&gwdt->wdd);
- return ret;
+}
+/* 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", },
So this is because we need to auto load the module if those resources are parsed from ACPI table, because there is no acpi_device_id for ACPI tables, right?
If so, please add related code in the ACPI patch, and comment on it please :)
Thanks Hanjun
On Friday 22 May 2015 22:50:30 Hanjun Guo wrote:
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTSBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, and why we depends on COMPILE_TEST?
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine, or run a 32-bit kernel on a chip that has it.
While SBSA requires this watchdog device, nothing prevents SoC manufacturers from using the same design in something that is not a server.
Arnd
On Fri, May 22, 2015 at 04:55:04PM +0200, Arnd Bergmann wrote:
On Friday 22 May 2015 22:50:30 Hanjun Guo wrote:
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTSBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, and why we depends on COMPILE_TEST?
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine, or run a 32-bit kernel on a chip that has it.
While SBSA requires this watchdog device, nothing prevents SoC manufacturers from using the same design in something that is not a server.
Tricky, though. Since teh driver uses arm specific clock functions, I don't think this can compile on a non-arm machine.
Guenter
On 2015年05月22日 23:01, Guenter Roeck wrote:
On Fri, May 22, 2015 at 04:55:04PM +0200, Arnd Bergmann wrote:
On Friday 22 May 2015 22:50:30 Hanjun Guo wrote:
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTSBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, and why we depends on COMPILE_TEST?
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine, or run a 32-bit kernel on a chip that has it.
While SBSA requires this watchdog device, nothing prevents SoC manufacturers from using the same design in something that is not a server.
From this point of view, I agree that SBSA watchdog design may used in other ARM SoCs in the future, but how about add it back when this kind of hardware showing up?
Tricky, though. Since teh driver uses arm specific clock functions, I don't think this can compile on a non-arm machine.
Since it depends on ARM64/ARM, we can temporary release from that now :)
Thanks Hanjun
On Friday 22 May 2015 23:18:21 Hanjun Guo wrote:
On 2015年05月22日 23:01, Guenter Roeck wrote:
On Fri, May 22, 2015 at 04:55:04PM +0200, Arnd Bergmann wrote:
On Friday 22 May 2015 22:50:30 Hanjun Guo wrote:
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTSBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, and why we depends on COMPILE_TEST?
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine, or run a 32-bit kernel on a chip that has it.
While SBSA requires this watchdog device, nothing prevents SoC manufacturers from using the same design in something that is not a server.
From this point of view, I agree that SBSA watchdog design may used in other ARM SoCs in the future, but how about add it back when this kind of hardware showing up?
If it builds on ARM32, I'd rather leave the option in, it doesn't hurt.
Tricky, though. Since teh driver uses arm specific clock functions, I don't think this can compile on a non-arm machine.
Since it depends on ARM64/ARM, we can temporary release from that now
We have to drop the '|| COMPILE_TEST' though as a result, or fix the driver to look up the clock in DT and call 'clk_get_rate'.
That will break the ACPI case, but ACPI could use platform_data to pass the clock rate into the driver, to make it independent of low-level APIs.
Arnd
On 05/22/2015 10:24 AM, Arnd Bergmann wrote:
That will break the ACPI case, but ACPI could use platform_data to pass the clock rate into the driver, to make it independent of low-level APIs.
The clock rate isn't the only problem. You still need to current clock timestamp, and that's ARM-specific.
On Friday 22 May 2015 11:19:52 Timur Tabi wrote:
On 05/22/2015 10:24 AM, Arnd Bergmann wrote:
That will break the ACPI case, but ACPI could use platform_data to pass the clock rate into the driver, to make it independent of low-level APIs.
The clock rate isn't the only problem. You still need to current clock timestamp, and that's ARM-specific.
Ah, I didn't realize that. I also just noticed that the two arch_timer functions we call here are inline functions that just access control registers.
If we want to enable COMPILE_TEST then, it would have to be something like
#ifdef CONFIG_ARM || CONFIG_ARM64 #include <asm/arch_timer.h> #else ... /* stub functions */ #endif
That might still be helpful to get coverage from things like the public coverity builds that always build x86 allmodconfig, but it's also a bit ugly.
Arnd
On 05/22/2015 03:13 PM, Arnd Bergmann wrote:
That might still be helpful to get coverage from things like the public coverity builds that always build x86 allmodconfig, but it's also a bit ugly.
Adding #ifdefs to allow COMPILE_TEST to work seems ironic to me.
Hi Arnd
Thanks :-)
On 22 May 2015 at 23:24, Arnd Bergmann arnd@arndb.de wrote:
On Friday 22 May 2015 23:18:21 Hanjun Guo wrote:
On 2015年05月22日 23:01, Guenter Roeck wrote:
On Fri, May 22, 2015 at 04:55:04PM +0200, Arnd Bergmann wrote:
On Friday 22 May 2015 22:50:30 Hanjun Guo wrote:
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTSBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, and why we depends on COMPILE_TEST?
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine, or run a 32-bit kernel on a chip that has it.
While SBSA requires this watchdog device, nothing prevents SoC manufacturers from using the same design in something that is not a server.
From this point of view, I agree that SBSA watchdog design may used in other ARM SoCs in the future, but how about add it back when this kind of hardware showing up?
If it builds on ARM32, I'd rather leave the option in, it doesn't hurt.
yes, I am agree with you. Reason: (1)Although the SBSA spec is all about ARMv8, but in "5 APPENDIX A: GENERIC WATCHDOG", nothing says SBSA watchdog is only for ARM64. (2)SBSA watchdog should work with arch_timer which has be used on ARMv7 and ARMv8. (3)there are ARM32 servers in the market(although there is not SBSA watchdog in them) (4) all the regs of SBSA watchdog are 32Bit, this IP core can be use in ARM32(I don't know the implementation detail of this IP core, but from the spec , I don't see "It can't be used on ARM32") (5)we don't need to change any code, any line.(yes, it doesn't hurt)
So maybe we can keep this
The only problem we face is : there is not a ARM32 hardware or a simulator of ARM32 has SBSA watchdog on it. and we don't know if ARM will put this IP core in any Soc in the future :-( Maybe because this IP core is new.
Tricky, though. Since teh driver uses arm specific clock functions, I don't think this can compile on a non-arm machine.
Since it depends on ARM64/ARM, we can temporary release from that now
We have to drop the '|| COMPILE_TEST' though as a result, or fix the driver to look up the clock in DT and call 'clk_get_rate'.
That will break the ACPI case, but ACPI could use platform_data to pass the clock rate into the driver, to make it independent of low-level APIs.
for this problem , we not only need to get "rate" but also need to get timestamp. So for now , we need to use some ARM specific code in arm_arch_timer.c, unless those interface is integrated into clk framework(or we have make a patch for if)
any suggestion or thought?
Great thanks for your review :-)
Arnd
Linaro-acpi mailing list Linaro-acpi@lists.linaro.org https://lists.linaro.org/mailman/listinfo/linaro-acpi
On 05/22/2015 10:01 AM, Guenter Roeck wrote:
Tricky, though. Since teh driver uses arm specific clock functions, I don't think this can compile on a non-arm machine.
Any it doesn't need to. This driver is for 64-bit ARM Server SOCs. There will never be any other kind of SOC that has an SBSA watchdog device in it.
Hi Guenter,
On 22 May 2015 at 23:01, Guenter Roeck linux@roeck-us.net wrote:
On Fri, May 22, 2015 at 04:55:04PM +0200, Arnd Bergmann wrote:
On Friday 22 May 2015 22:50:30 Hanjun Guo wrote:
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..25a0df1 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,18 @@ 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 ARM || ARM64 || COMPILE_TESTSBSA is for ARMv8-A based (64-bit) servers, no need to depends on ARM, and why we depends on COMPILE_TEST?
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine, or run a 32-bit kernel on a chip that has it.
While SBSA requires this watchdog device, nothing prevents SoC manufacturers from using the same design in something that is not a server.
Tricky, though. Since teh driver uses arm specific clock functions, I don't think this can compile on a non-arm machine.
yes, According to SBSA spec, the clock source of SBSAwatchdog is system counter which is a part of arm arch timer. So I think SBSA watchdog always goes with ARM.
ARM arch timer has been used on ARM32 and ARM64, so SBSA also can be used on ARM32. Just there is not a ARM32 hardware has that IP core right now.
Guenter
On 05/22/2015 09:55 AM, Arnd Bergmann wrote:
While SBSA requires this watchdog device, nothing prevents SoC manufacturers from using the same design in something that is not a server.
The first "S" in SBSA stands for "Server". I don't think it makes sense to put an SBSA watchdog device in a non-server SOC.
Frankly, I don't understand why people want to make this driver work on hardware that doesn't exist. The SBSA is intended for 64-bit ARM Servers that use ACPI. Yes, there are a couple of ARM64 servers that use device tree, but those are legacy platforms.
Arnd Bergmann wrote:
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine,
I'm going to have to disagree. If they haven't done it by now, I can't imagine any ARM SOC vendor creating a 32-bit ARM SOC with an SBSA watchdog in it. I can imagine a vendor trying to repurpose an existing 32-bit ARM SOC for the server market, but that SOC won't have an SBSA watchdog in it.
or run a 32-bit kernel on a chip that has it.
That might happen, but I would be very surprised, and I would need to be convinced that it's useful.
Hi Timur,
On 23 May 2015 at 23:08, Timur Tabi timur@codeaurora.org wrote:
Arnd Bergmann wrote:
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine,
I'm going to have to disagree. If they haven't done it by now, I can't imagine any ARM SOC vendor creating a 32-bit ARM SOC with an SBSA watchdog in it. I can imagine a vendor trying to repurpose an existing 32-bit ARM SOC for the server market, but that SOC won't have an SBSA watchdog in it.
I will agree with you on this, ONLY IF a people can represent ARM and all chip vendors say publicly: " We never ever use SBSA watchdog IP core on ARM32!" or " SBSA watchdog IP core is imcompatible with ARM32"
Although the SBSA is all about ARMv8, but in "5 APPENDIX A: GENERIC WATCHDOG", it doesn't say "this is only for ARMv8". and its clock source "system counter" and arm_arch_timer have been in ARM32 Soc for years, and all the regs in SBSA watchdog is 32bit. I can't see why we can not do that, unless I miss something.
I wonder why you are so sure "that SOC won't have an SBSA watchdog in it." any documentation ? Sorry, I am not a chip design engineer, I can't see why 32-bit ARM won't have an SBSA watchdog in it.
or run a 32-bit kernel on
a chip that has it.
That might happen, but I would be very surprised, and I would need to be convinced that it's useful.
-- 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 05/23/2015 10:26 AM, Fu Wei wrote:
Hi Timur,
On 23 May 2015 at 23:08, Timur Tabi timur@codeaurora.org wrote:
Arnd Bergmann wrote:
I think it's a reasonable assumption that someone will sooner or later put that hardware into an ARM32 machine,
I'm going to have to disagree. If they haven't done it by now, I can't imagine any ARM SOC vendor creating a 32-bit ARM SOC with an SBSA watchdog in it. I can imagine a vendor trying to repurpose an existing 32-bit ARM SOC for the server market, but that SOC won't have an SBSA watchdog in it.
I will agree with you on this, ONLY IF a people can represent ARM and all chip vendors say publicly: " We never ever use SBSA watchdog IP core on ARM32!" or " SBSA watchdog IP core is imcompatible with ARM32"
Although the SBSA is all about ARMv8, but in "5 APPENDIX A: GENERIC WATCHDOG", it doesn't say "this is only for ARMv8". and its clock source "system counter" and arm_arch_timer have been in ARM32 Soc for years, and all the regs in SBSA watchdog is 32bit. I can't see why we can not do that, unless I miss something.
I wonder why you are so sure "that SOC won't have an SBSA watchdog in it." any documentation ? Sorry, I am not a chip design engineer, I can't see why 32-bit ARM won't have an SBSA watchdog in it.
I think it is quite unfortunate that the specification is not public. We have heard many statements about what is in the spec or not. We have two possible implementations of the SBSA watchdog, but not even the implementers of those two implementations seem to be able to agree what the specification actually mandates/supports, or what it doesn't mandate/support.
We have heard that the SBSA watchdog would require ACPI, and that it doesn't. We have heard that it would require ARM64, and that it doesn't. We have heard various assumptions about how WS0 and WS1 are supposed to be wired. We have heard that all its registers are 32 bit (which would suggest they have to be treated as such), but then we have a 64 bit register access in one of the drivers, and a more complex implementation to read the same value as two 32-bit reads in the other. We have heard that WS0 and WS1 are at least to some degree independent of each other (and thus that it makes sense to set them separately), and we have heard that WS1 is always equal to WS0 * 2. We have one implementation limiting support to architecture revision 0, and the other not imposing such a restriction.
Is the specification really that vague in such key areas ? How do you expect anyone who doesn't have access to the specification to be able to figure out what is actually correct and how to proceed ?
Thanks, Guenter
Guenter Roeck wrote:
I think it is quite unfortunate that the specification is not public. We have heard many statements about what is in the spec or not.
All you need to do is go to http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.h..., get a free ARM account, and download the spec.
Hi Timur
On 24 May 2015 at 02:37, Timur Tabi timur@codeaurora.org wrote:
Guenter Roeck wrote:
I think it is quite unfortunate that the specification is not public. We have heard many statements about what is in the spec or not.
All you need to do is go to http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.h..., get a free ARM account, and download the spec.
Great thanks for providing this info!
-- 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 05/23/2015 11:37 AM, Timur Tabi wrote:
Guenter Roeck wrote:
I think it is quite unfortunate that the specification is not public. We have heard many statements about what is in the spec or not.
All you need to do is go to http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.h..., get a free ARM account, and download the spec.
That helps - thanks a lot!
Folks, please correct me if my understanding of the specification is wrong.
1) Pretimeout
The document suggests that WS1 = WS0 * 2 is in fact correct. In essence, there is just one counter, not two. This means that a separate pretimeout does not really make sense, since in practice the timeout would always be twice the pretimeout, and changing just one without affecting the other is not really possible.
2) 64 bit WCV register
The specification is not clear on how to read this register. It clearly states that it is comprised of two 32-bit registers, but it does not specify if a single 64-bit read would be atomic, or if it would be split into two separate 32-bit operations. This leaves room for interpretation by the implementer, and may result in bad values if the implementation changes the counter from, say, 0x000010000 to 0x0000ffff while the value is read.
My interpretation of this is that it would be safer to read two 32-bit values and ensure that the values are consistent instead of relying on the assumption that a single 64-bit read would be atomic.
3) ACPI vs. FDT
The specification does not say anything about ACPI or FDT support except that it assumes that there are "System description data structures such as ACPI or FDT". Given that, the driver should support both.
4) ARM vs. ARM64
SBSA clearly states that any CPU supporting it shall be ARM v8 compliant (4.1.1, CPU architecture). Personally I think the discussion around supporting the driver on ARM/ARM64 or ARM64 only is a bit pointless, especially since being able to build it on ARM doesn't really hurt, even though there is currently no HW supporting it.
Overall I must admit that I don't really understand why this is such a contentious issue.
5) Revision support
While it is difficult to predict the future, it is common practice in the industry to make future revisions of a standard (and chip) as much backward compatible as possible, and to only add functionality. As such, I don't see a reason to restrict support to revision 0 of the standard.
6) A note about driver messages
Implementation defined registers are just that, implementation defined registers. I don't think it makes sense to display any of those, not even for debugging purposes.
---
Again, please correct me if any of those statements is wrong. When doing so, please reference the specification, to make sure that we all know what we are talking about.
My hope is that we can use this as a starting point to converge on a single driver.
Thanks, Guenter
Hi Guenter,
Great thanks for your suggestion and review, I have some questions below , would you please help me out?
On 24 May 2015 at 03:51, Guenter Roeck linux@roeck-us.net wrote:
On 05/23/2015 11:37 AM, Timur Tabi wrote:
Guenter Roeck wrote:
I think it is quite unfortunate that the specification is not public. We have heard many statements about what is in the spec or not.
All you need to do is go to http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.h..., get a free ARM account, and download the spec.
That helps - thanks a lot!
Folks, please correct me if my understanding of the specification is wrong.
- Pretimeout
The document suggests that WS1 = WS0 * 2
Are you saying: the first timeout == the second timeout
|-------------WS0 |-------------WS1
Sorry, could you let me know where is that suggestion?? I have checked the SBSA again, but I can not find it. Maybe I really miss this part.
is in fact correct. In essence, there is just one counter, not two. This means that a separate pretimeout does not really make sense, since in practice the timeout would always be twice the pretimeout,
Yes, you are right, if we only use "WOR", then the first timeout == the second timeout
and changing just one without affecting the other is not really possible.
although there is only one counter, and it is 32 bits wide. In SBSA, we can see this: ------- Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register. ------- So for the first timeout, we can set the compare value register(WCV). Then the two timeouts are different. and the first timeout has not 10s(@400MHz) limit. just the the second timeout must use "WOR".
- 64 bit WCV register
The specification is not clear on how to read this register. It clearly states that it is comprised of two 32-bit registers, but it does not specify if a single 64-bit read would be atomic, or if it would be split into two separate 32-bit operations. This leaves room for interpretation by the implementer, and may result in bad values if the implementation changes the counter from, say, 0x000010000 to 0x0000ffff while the value is read.
My interpretation of this is that it would be safer to read two 32-bit values and ensure that the values are consistent instead of relying on the assumption that a single 64-bit read would be atomic.
yes, thanks for pointing it out.
I found this problem in my first upstream patchset. So in this patchset, there is not that problem now:
------- do { wcv_hi = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd); wcv_lo = sbsa_gwdt_cf_read(SBSA_GWDT_WCV_LO, wdd); } while (wcv_hi != sbsa_gwdt_cf_read(SBSA_GWDT_WCV_HI, wdd));
-------
- ACPI vs. FDT
The specification does not say anything about ACPI or FDT support except that it assumes that there are "System description data structures such as ACPI or FDT". Given that, the driver should support both.
yes, this patchset has fully support ACPI and FDT, and has been successfully tested on (1)Foundation model (2)AMD Seattle B0 Soc
- ARM vs. ARM64
SBSA clearly states that any CPU supporting it shall be ARM v8 compliant (4.1.1, CPU architecture). Personally I think the discussion around supporting the driver on ARM/ARM64 or ARM64 only is a bit pointless, especially since being able to build it on ARM doesn't really hurt, even though there is currently no HW supporting it.
Overall I must admit that I don't really understand why this is such a contentious issue.
I think that is not a contentious issue. Just some one has that suggestion, then we discussed it. So I am going to delete ARM support. If we have this hardware in the future, we can add it by then.
- Revision support
While it is difficult to predict the future, it is common practice in the industry to make future revisions of a standard (and chip) as much backward compatible as possible, and to only add functionality. As such, I don't see a reason to restrict support to revision 0 of the standard.
Totally agree, there is not this problem in my patchset.
- A note about driver messages
Implementation defined registers are just that, implementation defined registers. I don't think it makes sense to display any of those, not even for debugging purposes.
in this patchset, I have totally deleted all the debug message, only keep: (1)3 error messages (2)2 warning messages reason: 1. if system reset by watchdog, we need to inform user (WCS: store watchdog status, WCV: store the timestamp of the last reboot, maybe these can provide some basic info of reboot ) 2. if watchdog has been enabled before register, there is something wrong we need to inform user. (WCS: store watchdog status) (3)1 info message reason: if watchdog has been successfully registered, we log it.
Please let me know if there is any redundant massage, I will fix it.
Again, please correct me if any of those statements is wrong. When doing so, please reference the specification, to make sure that we all know what we are talking about.
Great thanks for your help , those help me a lot.
So, for now , this only big problem in my patchset is "pretimeout", but I have some doubt above, would you help me out ? :-) Thanks
My hope is that we can use this as a starting point to converge on a single driver.
Thanks, Guenter
On 05/24/2015 02:58 AM, Fu Wei wrote:
Hi Guenter,
Great thanks for your suggestion and review, I have some questions below , would you please help me out?
On 24 May 2015 at 03:51, Guenter Roeck linux@roeck-us.net wrote:
On 05/23/2015 11:37 AM, Timur Tabi wrote:
Guenter Roeck wrote:
I think it is quite unfortunate that the specification is not public. We have heard many statements about what is in the spec or not.
All you need to do is go to http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.h..., get a free ARM account, and download the spec.
That helps - thanks a lot!
Folks, please correct me if my understanding of the specification is wrong.
- Pretimeout
The document suggests that WS1 = WS0 * 2
Are you saying: the first timeout == the second timeout
|-------------WS0 |-------------WS1
Sorry, could you let me know where is that suggestion?? I have checked the SBSA again, but I can not find it. Maybe I really miss this part.
The document states:
"If both watchdog signals are deasserted and a timeout refresh occurs then WS0 is asserted. If WS0 is asserted and a timeout refresh occurs then WS1 is asserted."
There is no mention that programming both WOR and WCV would or even could result in different timeouts for the two watchdog signals.
is in fact correct. In essence, there is just one counter, not two. This means that a separate pretimeout does not really make sense, since in practice the timeout would always be twice the pretimeout,
Yes, you are right, if we only use "WOR", then the first timeout == the second timeout
and changing just one without affecting the other is not really possible.
although there is only one counter, and it is 32 bits wide. In SBSA, we can see this:
Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register.
So for the first timeout, we can set the compare value register(WCV). Then the two timeouts are different. and the first timeout has not 10s(@400MHz) limit. just the the second timeout must use "WOR".
That is not how I read the specification. It only talks about "timeout refresh". There is no indication that using both registers would have the impact you describe.
Since there is no mention of different WS0 and WS1 timeout periods in the specification, I am concerned that even _if_ a specific implementation supports such different timeouts, it would be implementation specific.
Maybe you and/or Timur can test this on the real systems you have access to. It should be quite straightforward to test - have the interrupt handler only print a message, program some values into the watchdog, and see when WS0 and WS1 occur.
Thanks, Guenter
Guenter Roeck wrote:
Maybe you and/or Timur can test this on the real systems you have access to. It should be quite straightforward to test - have the interrupt handler only print a message, program some values into the watchdog, and see when WS0 and WS1 occur.
I'll try, but on my system, WS1 is currently not implemented fully. Let me investigate and get back to you.
Hi Guenter,
Great thanks for your time, you provide your feedback in your weekend, I am so appreciate that.
my feedback inline below
On 24 May 2015 at 22:06, Guenter Roeck linux@roeck-us.net wrote:
On 05/24/2015 02:58 AM, Fu Wei wrote:
Hi Guenter,
Great thanks for your suggestion and review, I have some questions below , would you please help me out?
On 24 May 2015 at 03:51, Guenter Roeck linux@roeck-us.net wrote:
On 05/23/2015 11:37 AM, Timur Tabi wrote:
Guenter Roeck wrote:
I think it is quite unfortunate that the specification is not public. We have heard many statements about what is in the spec or not.
All you need to do is go to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.den0029b/index.h..., get a free ARM account, and download the spec.
That helps - thanks a lot!
Folks, please correct me if my understanding of the specification is wrong.
- Pretimeout
The document suggests that WS1 = WS0 * 2
Are you saying: the first timeout == the second timeout
|-------------WS0 |-------------WS1
Sorry, could you let me know where is that suggestion?? I have checked the SBSA again, but I can not find it. Maybe I really miss this part.
The document states:
"If both watchdog signals are deasserted and a timeout refresh occurs then WS0 is asserted. If WS0 is asserted and a timeout refresh occurs then WS1 is asserted."
yes, this just describe how the WS0/WS1 are deasserted or asserted. but it doesn't suggest WS1 = WS0 * 2. Of course, If we only config WOR in the driver, we get WS1 = WS0 * 2.
If we delete pretimeout concept from this driver Then what is the timeout, |-------------WS0 or |-------------WS0-------------WS1 ?
If we only config WOR , the timeout for two stage will be the same I think the first stage should be called timeout, the user get a panic in timeout(the first stage) Then we can tell user, in this driver, if first timeout is triggered but fail, we can wait for the same time for reboot.
but the second timeout is still like a pretimeout, why not just use it ?
IMO, the key point of SBSA watchdog is two signal, and the first one is interrupt, the second one maybe a hardware reset OR a interrupt if we drop "pretimeout", the watchdog just acts like a common watchdog (just like SP805)
Now we assume that the second signal is a reset, but according to SBSA, that maybe a interrupt. If that happens on a Soc(that will happen), how to deal with that? what is the timeout between WS0 and WS1?
If the two signals are all connect to interrupts, I think we still need a pretimeout. why not use pretimeout right now. that will be more easy to maintain the driver.
There is no mention that programming both WOR and WCV would or even could result in different timeouts for the two watchdog signals.
yes, they don't mention it. using WCV to config the first stage and WOR for the second stage is my idea to solve the two stage timeouts. Because by this way, we can make the first stage longer(has not that 10s(@400MHz) limit), and we can provide the full feature of SBSA watchdog. as I have mentioned above, If the two signals are all connect to interrupts, we don't need to tell user : the two stage timeout must be the same and 10s(@400MHz) limit,
is in fact correct. In essence, there is just one counter, not two. This means that a separate pretimeout does not really make sense, since in practice the timeout would always be twice the pretimeout,
Yes, you are right, if we only use "WOR", then the first timeout == the second timeout
and changing just one without affecting the other is not really possible.
although there is only one counter, and it is 32 bits wide. In SBSA, we can see this:
Note: the watchdog offset register is 32 bits wide. This gives a maximum watch period of around 10s at a system counter frequency of 400MHz. If a larger watch period is required then the compare value can be programmed directly into the compare value register.
So for the first timeout, we can set the compare value register(WCV). Then the two timeouts are different. and the first timeout has not 10s(@400MHz) limit. just the the second timeout must use "WOR".
That is not how I read the specification. It only talks about "timeout refresh". There is no indication that using both registers would have the impact you describe.
Since there is no mention of different WS0 and WS1 timeout periods in the specification, I am concerned that even _if_ a specific implementation supports such different timeouts, it would be implementation specific.
yes, you are right, they don't mention, but they can be different, that is the way I use. I just put limit on user , say: the two timeout must be the same. I hope my driver can provide the full feature of hardware. but they make driver a little more complicated than the one which simply config WOR. I have to say , another one make SBSA watchdog become a watchdog.
Maybe you and/or Timur can test this on the real systems you have access to. It should be quite straightforward to test - have the interrupt handler only print a message, program some values into the watchdog, and see when WS0 and WS1 occur.
yes, the first version of my driver has printed the timeleft out, I can have different timeouts and pretimeout
Thanks, Guenter
Fu Wei wrote:
I wonder why you are so sure "that SOC won't have an SBSA watchdog in it." any documentation ? Sorry, I am not a chip design engineer, I can't see why 32-bit ARM won't have an SBSA watchdog in it.
Because there's no market for it. I'm not talking about what's theoretically possible. I'm only talking about what makes sense and what will actually happen. And I'm quite certain that we will never see an actual 32-bit ARM SOC with an SBSA watchdog device in it.
Therefore, it makes no sense to complicated the code so that we can support an SOC that will never exist.
So can we PLEASE stop talking about 32-bit ARM support?
Hi Timur
On 24 May 2015 at 02:40, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
I wonder why you are so sure "that SOC won't have an SBSA watchdog in it." any documentation ? Sorry, I am not a chip design engineer, I can't see why 32-bit ARM won't have an SBSA watchdog in it.
Because there's no market for it. I'm not talking about what's theoretically possible. I'm only talking about what makes sense and what will actually happen. And I'm quite certain that we will never see an actual 32-bit ARM SOC with an SBSA watchdog device in it.
Why are you quite certain? any info you can kindly share here?
Therefore, it makes no sense to complicated the code so that we can support an SOC that will never exist.
yes, that is a good reason!
So can we PLEASE stop talking about 32-bit ARM support?
why? we are just trying to figure out: Do we need to add ARM in "depends on" for SBSA watchdog driver?
If some one suggests this, we need to figure out.
-- 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.
Fu Wei wrote:
Because there's no market for it. I'm not talking about what's theoretically possible. I'm only talking about what makes sense and what will actually happen. And I'm quite certain that we will never see an actual 32-bit ARM SOC with an SBSA watchdog device in it.
Why are you quite certain? any info you can kindly share here?
I used to work for a company called Calxeda that tried to sell a 32-bit ARM Server SOC. Calxeda went out of business because no one would buy it, and that was two years ago.
So can we PLEASE stop talking about 32-bit ARM support?
why? we are just trying to figure out: Do we need to add ARM in "depends on" for SBSA watchdog driver?
If some one suggests this, we need to figure out.
We do not need to add "depends on ARM" in the Kconfig for the SBSA watchdog driver.
From: Fu Wei fu.wei@linaro.org
Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by the ARM SBSA Generic Watchdog driver.
Signed-off-by: Fu Wei fu.wei@linaro.org --- arch/arm64/kernel/acpi.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..1ed11fd 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -23,6 +23,7 @@ #include <linux/irqdomain.h> #include <linux/memblock.h> #include <linux/of_fdt.h> +#include <linux/platform_device.h> #include <linux/smp.h>
#include <asm/cputype.h> @@ -343,3 +344,138 @@ void __init acpi_gic_init(void)
early_acpi_os_unmap_memory((char *)table, tbl_size); } + +static int __init acpi_gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd, + int index) +{ + struct platform_device *pdev; + struct resource *res; + u32 gsi, flags; + int irq, trigger, polarity; + resource_size_t rf_base_phy, cf_base_phy; + int err = -ENOMEM; + + /* + * Get SBSA Generic Watchdog info + * from a Watchdog GT type structure in GTDT + */ + rf_base_phy = (resource_size_t)wd->refresh_frame_address; + cf_base_phy = (resource_size_t)wd->control_frame_address; + gsi = wd->timer_interrupt; + flags = wd->timer_flags; + + pr_info("GTDT: a Watchdog GT structure(0x%llx/0x%llx gsi:%u flags:0x%x)\n", + rf_base_phy, cf_base_phy, gsi, flags); + + if (!(rf_base_phy && cf_base_phy && gsi)) { + pr_err("GTDT: failed geting the device info.\n"); + return -EINVAL; + } + + trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE + : ACPI_LEVEL_SENSITIVE; + polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW + : ACPI_ACTIVE_HIGH; + irq = acpi_register_gsi(NULL, gsi, trigger, polarity); + if (irq < 0) { + pr_err("GTDT: failed to register GSI of the Watchdog GT.\n"); + return -EINVAL; + } + + pdev = platform_device_alloc("sbsa-gwdt", index); + if (!pdev) + goto err_unregister_gsi; + + res = kcalloc(3, sizeof(*res), GFP_KERNEL); + if (!res) + goto err_device_put; + + res[0].start = rf_base_phy; + res[0].end = rf_base_phy + SZ_4K - 1; + res[0].name = "refresh"; + res[0].flags = IORESOURCE_MEM; + + res[1].start = cf_base_phy; + res[1].end = cf_base_phy + SZ_4K - 1; + res[1].name = "control"; + res[1].flags = IORESOURCE_MEM; + + res[2].start = irq; + res[2].end = res[2].start; + res[2].name = "ws0"; + res[2].flags = IORESOURCE_IRQ; + + err = platform_device_add_resources(pdev, res, 3); + if (err) + goto err_free_res; + + err = platform_device_add(pdev); + if (err) + goto err_free_res; + + return 0; + +err_free_res: + kfree(res); +err_device_put: + platform_device_put(pdev); +err_unregister_gsi: + acpi_unregister_gsi(gsi); + + return err; +} + +/* Initialize SBSA generic Watchdog platform device info from GTDT */ +static int __init acpi_gtdt_sbsa_gwdt_init(struct acpi_table_header *table) +{ + struct acpi_table_gtdt *gtdt; + struct acpi_gtdt_header *header; + void *gtdt_subtable; + int i, gwdt_index; + int ret = 0; + + if (table->revision < 2) { + pr_info("GTDT: Revision:%d doesn't support Platform Timers.\n", + table->revision); + return 0; + } + + gtdt = container_of(table, struct acpi_table_gtdt, header); + if (!gtdt->platform_timer_count) { + pr_info("GTDT: No Platform Timer structures.\n"); + return 0; + } + + gtdt_subtable = (void *)gtdt + gtdt->platform_timer_offset; + + for (i = 0, gwdt_index = 0; i < gtdt->platform_timer_count; i++) { + if (gtdt_subtable > (void *)table + table->length) { + pr_err("GTDT: subtable pointer overflows, bad table\n"); + return -EINVAL; + } + header = (struct acpi_gtdt_header *)gtdt_subtable; + if (header->type == ACPI_GTDT_TYPE_WATCHDOG) { + ret = acpi_gtdt_import_sbsa_gwdt(gtdt_subtable, + gwdt_index); + if (ret) + pr_err("GTDT: failed to import subtable %d\n", + i); + else + gwdt_index++; + } + gtdt_subtable += header->length; + } + + return ret; +} + +/* Initialize the SBSA Generic Watchdog presented in GTDT */ +static int __init acpi_gtdt_init(void) +{ + if (acpi_disabled) + return 0; + + return acpi_table_parse(ACPI_SIG_GTDT, acpi_gtdt_sbsa_gwdt_init); +} + +arch_initcall(acpi_gtdt_init);
+CC Catalin and Will
On 2015年05月21日 16:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by the ARM SBSA Generic Watchdog driver.
Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/kernel/acpi.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..1ed11fd 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -23,6 +23,7 @@ #include <linux/irqdomain.h> #include <linux/memblock.h> #include <linux/of_fdt.h> +#include <linux/platform_device.h> #include <linux/smp.h>
#include <asm/cputype.h> @@ -343,3 +344,138 @@ void __init acpi_gic_init(void)
early_acpi_os_unmap_memory((char *)table, tbl_size); }
+static int __init acpi_gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd,
int index)+{
- struct platform_device *pdev;
- struct resource *res;
- u32 gsi, flags;
- int irq, trigger, polarity;
- resource_size_t rf_base_phy, cf_base_phy;
- int err = -ENOMEM;
- /*
* Get SBSA Generic Watchdog info* from a Watchdog GT type structure in GTDT*/- rf_base_phy = (resource_size_t)wd->refresh_frame_address;
- cf_base_phy = (resource_size_t)wd->control_frame_address;
- gsi = wd->timer_interrupt;
- flags = wd->timer_flags;
- pr_info("GTDT: a Watchdog GT structure(0x%llx/0x%llx gsi:%u flags:0x%x)\n",
rf_base_phy, cf_base_phy, gsi, flags);
Can we use pr_debug here? I don't think those information worthy a pr_info.
- if (!(rf_base_phy && cf_base_phy && gsi)) {
pr_err("GTDT: failed geting the device info.\n");return -EINVAL;- }
- trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
: ACPI_LEVEL_SENSITIVE;- polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
: ACPI_ACTIVE_HIGH;
I see places to duplicate this, I will look into this to see we can form a function to handle it.
- irq = acpi_register_gsi(NULL, gsi, trigger, polarity);
- if (irq < 0) {
pr_err("GTDT: failed to register GSI of the Watchdog GT.\n");return -EINVAL;- }
- pdev = platform_device_alloc("sbsa-gwdt", index);
Please put a comment before this function to explain why we need a "sbsa-gwdt" name here.
- if (!pdev)
goto err_unregister_gsi;- res = kcalloc(3, sizeof(*res), GFP_KERNEL);
- if (!res)
goto err_device_put;- res[0].start = rf_base_phy;
- res[0].end = rf_base_phy + SZ_4K - 1;
- res[0].name = "refresh";
- res[0].flags = IORESOURCE_MEM;
- res[1].start = cf_base_phy;
- res[1].end = cf_base_phy + SZ_4K - 1;
So why SZ_4K? is it defined in SBSA spec? if so, please comment on it too.
- res[1].name = "control";
- res[1].flags = IORESOURCE_MEM;
- res[2].start = irq;
- res[2].end = res[2].start;
- res[2].name = "ws0";
- res[2].flags = IORESOURCE_IRQ;
- err = platform_device_add_resources(pdev, res, 3);
- if (err)
goto err_free_res;- err = platform_device_add(pdev);
- if (err)
goto err_free_res;- return 0;
+err_free_res:
- kfree(res);
+err_device_put:
- platform_device_put(pdev);
+err_unregister_gsi:
- acpi_unregister_gsi(gsi);
- return err;
+}
+/* Initialize SBSA generic Watchdog platform device info from GTDT */ +static int __init acpi_gtdt_sbsa_gwdt_init(struct acpi_table_header *table) +{
- struct acpi_table_gtdt *gtdt;
- struct acpi_gtdt_header *header;
- void *gtdt_subtable;
- int i, gwdt_index;
- int ret = 0;
- if (table->revision < 2) {
pr_info("GTDT: Revision:%d doesn't support Platform Timers.\n",table->revision);
pr_warn() would be good.
Thanks Hanjun
fu.wei@linaro.org wrote:
From: Fu Weifu.wei@linaro.org
Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by the ARM SBSA Generic Watchdog driver.
Signed-off-by: Fu Weifu.wei@linaro.org
Tested-by: Timur Tabi timur@codeaurora.org
I have modified my watchdog driver to use this patch.
On Thursday 21 May 2015 16:32:29 fu.wei@linaro.org wrote:
This patchset:
(1)Export "arch_timer_get_rate" in arm_arch_timer.c for the other drivers, like SBSA watchdog driver (2)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. (3)Introduce "pretimeout" into the watchdog framework, and update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts". (4)Introduce ARM SBSA watchdog driver: a.Use linux kernel watchdog framework; b.Work with FDT on ARM64; c.Use "pretimeout" in watchdog framework; d.In first timeout(WS0), do panic to save system context; e.Support geting timeout and pretimeout from parameter and FDT at the driver init stage.
Looks all good to me, please add
Acked-by: Arnd Bergmann arnd@arndb.de
for patches 1-6.
(5)Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by This Watchdog driver.
This needs to be reviewed by someone who understands ACPI.
Arnd
Hi Arnd,
Great thanks for your review! :-)
On 21 May 2015 at 16:46, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 21 May 2015 16:32:29 fu.wei@linaro.org wrote:
This patchset:
(1)Export "arch_timer_get_rate" in arm_arch_timer.c for the other drivers, like SBSA watchdog driver (2)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. (3)Introduce "pretimeout" into the watchdog framework, and update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts". (4)Introduce ARM SBSA watchdog driver: a.Use linux kernel watchdog framework; b.Work with FDT on ARM64; c.Use "pretimeout" in watchdog framework; d.In first timeout(WS0), do panic to save system context; e.Support geting timeout and pretimeout from parameter and FDT at the driver init stage.Looks all good to me, please add
Acked-by: Arnd Bergmann arnd@arndb.de
for patches 1-6.
Great! happy to do so :-)
(5)Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by This Watchdog driver.This needs to be reviewed by someone who understands ACPI.
yes, maybe Hanjun and Al can help :-)
Arnd
For patch 1,4,5,6,and 7, I have tested this on AMD Seattle platform.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
Thanks,
Suravee
On 5/21/15 03:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This patchset:
(1)Export "arch_timer_get_rate" in arm_arch_timer.c for the other drivers, like SBSA watchdog driver (2)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. (3)Introduce "pretimeout" into the watchdog framework, and update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_ops struct; (2)the new API "watchdog_init_timeouts". (4)Introduce ARM SBSA watchdog driver: a.Use linux kernel watchdog framework; b.Work with FDT on ARM64; c.Use "pretimeout" in watchdog framework; d.In first timeout(WS0), do panic to save system context; e.Support geting timeout and pretimeout from parameter and FDT at the driver init stage. (5)Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by This Watchdog driver.This patchset has been tested with watchdog daemon (ACPI/FDT, module/build-in) on the following platforms: (1)ARM Foundation v8 model (2)AMD Seattle B0
Changelog: v2: 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: The first version upstream patchset to linux mailing list
Fu Wei (7): clocksource: export "arch_timer_get_rate" for the other drivers Documentation: add sbsa-gwdt.txt 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 "pretimeout" into framework Watchdog: introduce ARM SBSA watchdog driver ACPI: import watchdog info of GTDT into platform device
.../devicetree/bindings/watchdog/sbsa-gwdt.txt | 36 ++ Documentation/watchdog/watchdog-kernel-api.txt | 62 ++- arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 11 + arch/arm64/boot/dts/arm/foundation-v8.dts | 10 + arch/arm64/kernel/acpi.c | 136 ++++++ drivers/clocksource/arm_arch_timer.c | 1 + drivers/watchdog/Kconfig | 12 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++ drivers/watchdog/watchdog_core.c | 103 +++-- drivers/watchdog/watchdog_dev.c | 48 +++ include/linux/watchdog.h | 30 +- 12 files changed, 891 insertions(+), 35 deletions(-) create mode 100644 Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt create mode 100644 drivers/watchdog/sbsa_gwdt.c
Hi Suravee,
Great thanks for your test, will add your "Tested-by:" in these patches, if I don't modify them in the next version
On 22 May 2015 at 04:36, Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com wrote:
For patch 1,4,5,6,and 7, I have tested this on AMD Seattle platform.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com
Thanks,
Suravee
On 5/21/15 03:32, fu.wei@linaro.org wrote:
From: Fu Wei fu.wei@linaro.org
This patchset:
(1)Export "arch_timer_get_rate" in arm_arch_timer.c for the other drivers, like SBSA watchdog driver (2)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. (3)Introduce "pretimeout" into the watchdog framework, and update Documentation/watchdog/watchdog-kernel-api.txt to introduce: (1)the new elements in the watchdog_device and watchdog_opsstruct; (2)the new API "watchdog_init_timeouts".
(4)Introduce ARM SBSA watchdog driver: a.Use linux kernel watchdog framework; b.Work with FDT on ARM64; c.Use "pretimeout" in watchdog framework; d.In first timeout(WS0), do panic to save system context; e.Support geting timeout and pretimeout from parameter and FDT at the driver init stage. (5)Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, and create a platform device with that information. This platform device can be used by This Watchdog driver.This patchset has been tested with watchdog daemon (ACPI/FDT, module/build-in) on the following platforms: (1)ARM Foundation v8 model (2)AMD Seattle B0
Changelog: v2: 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: The first version upstream patchset to linux mailing list
Fu Wei (7): clocksource: export "arch_timer_get_rate" for the other drivers Documentation: add sbsa-gwdt.txt 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 "pretimeout" into framework Watchdog: introduce ARM SBSA watchdog driver ACPI: import watchdog info of GTDT into platform device
.../devicetree/bindings/watchdog/sbsa-gwdt.txt | 36 ++ Documentation/watchdog/watchdog-kernel-api.txt | 62 ++- arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 11 + arch/arm64/boot/dts/arm/foundation-v8.dts | 10 + arch/arm64/kernel/acpi.c | 136 ++++++ drivers/clocksource/arm_arch_timer.c | 1 + drivers/watchdog/Kconfig | 12 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 476 +++++++++++++++++++++ drivers/watchdog/watchdog_core.c | 103 +++-- drivers/watchdog/watchdog_dev.c | 48 +++ include/linux/watchdog.h | 30 +- 12 files changed, 891 insertions(+), 35 deletions(-) create mode 100644 Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt create mode 100644 drivers/watchdog/sbsa_gwdt.c
-
Arnd Bergmann -
Fu Wei -
fu.wei＠linaro.org
-
Guenter Roeck -
Hanjun Guo -
Suravee Suthikulpanit -
Timo Kokkonen -
Timur Tabi