From: Fu Wei fu.wei@linaro.org
This patchset: (1)Introduce Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt for FDT info of SBSA Generic Watchdog, and give two examples of adding SBSA Generic Watchdog device node into the dts files: foundation-v8.dts and amd-seattle-soc.dtsi.
(2)Introduce "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".
(3)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, do panic to save system context; e.Support getting timeout and pretimeout from parameter and FDT at the driver init stage.
(4)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: v3: Delete "export arch_timer_get_rate" patch. Driver back to use arch_timer_get_cntfrq. Improve watchdog_init_timeouts function and update relevant documentation. Improve watchdog_timeout_invalid and watchdog_pretimeout_invalid. Improve foundation-v8.dts: delete the unnecessary tag of device node. Remove "ARM64 || COMPILE_TEST" from Kconfig. Add comments in arch/arm64/kernel/acpi.c Fix typoes and incorrect comments.
v2: 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 (6): 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: introdouce "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 | 47 +- arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 11 + arch/arm64/boot/dts/arm/foundation-v8.dts | 10 + arch/arm64/kernel/acpi.c | 145 +++++++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 474 +++++++++++++++++++++ drivers/watchdog/watchdog_core.c | 95 +++-- drivers/watchdog/watchdog_dev.c | 50 +++ include/linux/watchdog.h | 33 +- 11 files changed, 880 insertions(+), 33 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
The sbsa-gwdt.txt documentation in devicetree/bindings/watchdog is for introducing SBSA(Server Base System Architecture) Generic Watchdog device node info into FDT.
Acked-by: Arnd Bergmann arnd@arndb.de 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..010e5c4 --- /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@2a440000 { + compatible = "arm,sbsa-gwdt"; + reg = <0x0 0x2a440000 0 0x10000>, + <0x0 0x2a450000 0 0x10000>; + reg-names = "control", "refresh"; + 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.
Acked-by: Arnd Bergmann arnd@arndb.de 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..962a07e 100644 --- a/arch/arm64/boot/dts/arm/foundation-v8.dts +++ b/arch/arm64/boot/dts/arm/foundation-v8.dts @@ -237,4 +237,14 @@ }; }; }; + watchdog@2a440000 { + compatible = "arm,sbsa-gwdt"; + reg = <0x0 0x2a440000 0 0x10000>, + <0x0 0x2a450000 0 0x10000>; + reg-names = "control", + "refresh"; + 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.
Acked-by: Arnd Bergmann arnd@arndb.de Acked-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Signed-off-by: Fu Wei fu.wei@linaro.org --- arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi | 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";
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 drivers are going to use this: ARM SBSA Generic Watchdog
Acked-by: Arnd Bergmann arnd@arndb.de Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Signed-off-by: Fu Wei fu.wei@linaro.org --- Documentation/watchdog/watchdog-kernel-api.txt | 47 +++++++++++-- drivers/watchdog/watchdog_core.c | 95 +++++++++++++++++++------- drivers/watchdog/watchdog_dev.c | 50 ++++++++++++++ include/linux/watchdog.h | 33 ++++++++- 4 files changed, 192 insertions(+), 33 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index a0438f3..95b355d 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. @@ -219,8 +236,28 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd, unsigned int timeout_parm, 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. +using the module timeout parameter or by retrieving the first element of +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. +This routine returns zero on success and a negative errno code for failure. + +Some watchdog timers have two stage of timeouts(timeout and pretimeout), +to initialize the timeout and pretimeout fields at the same time, the following +function can be used: + +extern int watchdog_init_timeouts(struct watchdog_device *wdd, + unsigned int pretimeout_parm, + unsigned int timeout_parm, + struct device *dev); + +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). +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. diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index cec9b55..85b1d33 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -43,60 +43,105 @@ 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, + struct device *dev) { - unsigned int t = 0; - int ret = 0; + u32 timeouts[2]; + int ret = 0, length = 0;
- 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; + else + ret = -EINVAL; } - if (timeout_parm) + + if (timeout_parm) { + if (!watchdog_timeout_invalid(wdd, timeout_parm)) { + wdd->timeout = timeout_parm; + return ret; + } 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 + + of_find_property(dev->of_node, "timeout-sec", &length); + if (length > 0 && length <= sizeof(u32) * 2) { + of_property_read_u32_array(dev->of_node, + "timeout-sec", timeouts, + length / sizeof(u32)); + if (length == 2) { + if (!watchdog_pretimeout_invalid(wdd, timeouts[1]) && + timeouts[1]) + wdd->pretimeout = timeouts[1]; + else + ret = -EINVAL; + } + + if (!watchdog_timeout_invalid(wdd, timeouts[0]) && timeouts[0]) + wdd->timeout = timeouts[0]; + 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 +164,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..a65a9b0 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,24 @@ 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: + /* pretimeout == 0 means that we don't know 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..e776e1d 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; @@ -117,7 +125,17 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t) { return ((wdd->max_timeout != 0) && - (t < wdd->min_timeout || t > wdd->max_timeout)); + (t < wdd->min_timeout || t > wdd->max_timeout || + t < wdd->pretimeout)); +} + +/* 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 || + (wdd->timeout != 0 && t > wdd->timeout))); }
/* Use the following functions to manipulate watchdog driver specific data */ @@ -132,11 +150,20 @@ 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, + 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, dev); +} + #ifdef CONFIG_HARDLOCKUP_DETECTOR void watchdog_nmi_disable_all(void); void watchdog_nmi_enable_all(void);
On 05/25/2015 03:03 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 drivers are going to use this: ARM SBSA Generic Watchdog
Acked-by: Arnd Bergmann arnd@arndb.de Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Signed-off-by: Fu Wei fu.wei@linaro.org
Comments inline.
Thanks, Guenter
Documentation/watchdog/watchdog-kernel-api.txt | 47 +++++++++++-- drivers/watchdog/watchdog_core.c | 95 +++++++++++++++++++------- drivers/watchdog/watchdog_dev.c | 50 ++++++++++++++ include/linux/watchdog.h | 33 ++++++++- 4 files changed, 192 insertions(+), 33 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index a0438f3..95b355d 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
@@ -219,8 +236,28 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd, unsigned int timeout_parm, 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. +using the module timeout parameter or by retrieving the first element of +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. +This routine returns zero on success and a negative errno code for failure.
+Some watchdog timers have two stage of timeouts(timeout and pretimeout), +to initialize the timeout and pretimeout fields at the same time, the following +function can be used:
+extern int watchdog_init_timeouts(struct watchdog_device *wdd,
unsigned int pretimeout_parm,
unsigned int timeout_parm,
struct device *dev);
+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). +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. diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index cec9b55..85b1d33 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -43,60 +43,105 @@ 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;
I think you should rather update max_pretimeout in this case, to be < max_timeout.
} }
/**
- 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,
{struct device *dev)
- unsigned int t = 0;
- int ret = 0;
- u32 timeouts[2];
- int ret = 0, length = 0;
Please keep the longest variable chain first.
- 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;
else
}ret = -EINVAL;
- if (timeout_parm)
- if (timeout_parm) {
if (!watchdog_timeout_invalid(wdd, timeout_parm)) {
wdd->timeout = timeout_parm;
return ret;
This would return -EINVAL if timeout is correct but pretimeout isn't. How about this ?
if (!ret) return 0;
}
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
- of_find_property(dev->of_node, "timeout-sec", &length);
- if (length > 0 && length <= sizeof(u32) * 2) {
of_property_read_u32_array(dev->of_node,
"timeout-sec", timeouts,
length / sizeof(u32));
if (length == 2) {
if (!watchdog_pretimeout_invalid(wdd, timeouts[1]) &&
timeouts[1])
wdd->pretimeout = timeouts[1];
A pretimeout of 0 is a valid timeout, so why not use it ?
else
ret = -EINVAL;
and why is pretimeout == 0 considered invalid here ?
}
if (!watchdog_timeout_invalid(wdd, timeouts[0]) && timeouts[0])
wdd->timeout = timeouts[0];
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 +164,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..a65a9b0 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,24 @@ 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
s/keep's/keeps/
* possible that it takes the new timeout)
new pretimeout
*/
watchdog_ping(wdd);
/* Fall */
- case WDIOC_GETPRETIMEOUT:
/* pretimeout == 0 means that we don't know the pretimeout */
if (wdd->pretimeout == 0)
return -EOPNOTSUPP;
pretimeout == 0 is a valid parameter, though, and means that it is disabled. So we can not return -EOPNOTSUPP here.
Either this code needs to explicitly check for pretimeout support, or just return 0 if it is disabled / not supported.
case WDIOC_GETTIMELEFT: err = watchdog_get_timeleft(wdd, &val); if (err)return put_user(wdd->pretimeout, p);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index a746bf5..e776e1d 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;
@@ -117,7 +125,17 @@ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool noway static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t) { return ((wdd->max_timeout != 0) &&
(t < wdd->min_timeout || t > wdd->max_timeout));
(t < wdd->min_timeout || t > wdd->max_timeout ||
t < wdd->pretimeout));
< or <= ?
timeout == pretimeout implies that the pretimeout would expire immediately, which doesn't seem to make much sense.
+}
+/* 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 ||
(wdd->timeout != 0 && t > wdd->timeout)));
> or >= ?
}
/* Use the following functions to manipulate watchdog driver specific data */ @@ -132,11 +150,20 @@ 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,
extern int watchdog_register_device(struct watchdog_device *); extern void watchdog_unregister_device(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, dev);
+}
- #ifdef CONFIG_HARDLOCKUP_DETECTOR void watchdog_nmi_disable_all(void); void watchdog_nmi_enable_all(void);
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, the interrupt routine run panic to save system context.
Acked-by: Arnd Bergmann arnd@arndb.de Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Signed-off-by: Fu Wei fu.wei@linaro.org --- drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 474 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 486 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..554f18a 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,17 @@ config ARM_SP805_WATCHDOG ARM Primecell SP805 Watchdog timer. This will reboot your system when the timeout is reached.
+config ARM_SBSA_WATCHDOG + tristate "ARM SBSA Generic Watchdog" + depends on ARM64 + depends on ARM_ARCH_TIMER + select WATCHDOG_CORE + help + ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts. + The first timeout will trigger a panic; the second timeout will + trigger a system reset. + More details: ARM 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..0d1aff1 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,474 @@ +/* + * 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: ARM DEN0029B - Server Base System Architecture (SBSA) + * + * Kernel/API: P---------| pretimeout + * |-------------------------------T timeout + * SBSA GWDT: P--WOR---WS1 pretimeout + * |-------WCV----------WS0~~~~~~~~T timeout + */ + +#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/uaccess.h> +#include <linux/watchdog.h> +#include <asm/arch_timer.h> + +/* SBSA Generic Watchdog register definitions */ +/* refresh frame */ +#define SBSA_GWDT_WRR 0x000 + +/* control frame */ +#define SBSA_GWDT_WCS 0x000 +#define SBSA_GWDT_WOR 0x008 +#define SBSA_GWDT_WCV_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_param; +module_param(timeout_param, uint, 0); +MODULE_PARM_DESC(timeout_param, + "Watchdog timeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_TIMEOUT) ")"); + +static unsigned int max_timeout_param = UINT_MAX; +module_param(max_timeout_param, uint, 0); +MODULE_PARM_DESC(max_timeout_param, + "Watchdog max timeout in seconds. (>=0, default=" + __MODULE_STRING(UINT_MAX) ")"); + +static unsigned int pretimeout_param; +module_param(pretimeout_param, uint, 0); +MODULE_PARM_DESC(pretimeout_param, + "Watchdog pretimeout in seconds. (>=0, default=" + __MODULE_STRING(DEFAULT_PRETIMEOUT) ")"); + +static unsigned int max_pretimeout_param = U32_MAX; +module_param(max_pretimeout_param, uint, 0); +MODULE_PARM_DESC(max_pretimeout_param, + "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) ")"); + +/* + * help functions for accessing 32bit registers of SBSA Generic Watchdog + */ +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 functions 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 update the 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(); + + 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; + + /* + * Get the frequency of system counter from + * the cp15 interface of ARM Generic timer + */ + clk = arch_timer_get_cntfrq(); + 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(cf_base)) + return PTR_ERR(cf_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_param); + wdd->min_timeout = 1; + do_div(first_period_max, clk); + wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max), + max_timeout_param); + + wdd->pretimeout = DEFAULT_PRETIMEOUT; + wdd->timeout = DEFAULT_TIMEOUT; + watchdog_init_timeouts(wdd, pretimeout_param, timeout_param, 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/25/2015 03:03 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, the interrupt routine run panic to save system context.
Acked-by: Arnd Bergmann arnd@arndb.de Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Signed-off-by: Fu Wei fu.wei@linaro.org
Comments inline.
Thanks, Guenter
drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 474 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 486 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..554f18a 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,17 @@ config ARM_SP805_WATCHDOG ARM Primecell SP805 Watchdog timer. This will reboot your system when the timeout is reached.
+config ARM_SBSA_WATCHDOG
- tristate "ARM SBSA Generic Watchdog"
- depends on ARM64
- depends on ARM_ARCH_TIMER
- select WATCHDOG_CORE
- help
ARM SBSA Generic Watchdog. This watchdog has two Watchdog timeouts.
The first timeout will trigger a panic; the second timeout will
trigger a system reset.
More details: ARM 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..0d1aff1 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,474 @@ +/*
- 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: ARM DEN0029B - Server Base System Architecture (SBSA)
- Kernel/API: P---------| pretimeout
|-------------------------------T timeout
- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout
- */
+#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/uaccess.h> +#include <linux/watchdog.h> +#include <asm/arch_timer.h>
+/* SBSA Generic Watchdog register definitions */ +/* refresh frame */ +#define SBSA_GWDT_WRR 0x000
+/* control frame */ +#define SBSA_GWDT_WCS 0x000 +#define SBSA_GWDT_WOR 0x008 +#define SBSA_GWDT_WCV_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*/
That is a bit low for a default. Is that on purpose ? Most drivers use 30 or 60 seconds.
+#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/
+static unsigned int timeout_param; +module_param(timeout_param, uint, 0); +MODULE_PARM_DESC(timeout_param,
"Watchdog timeout in seconds. (>=0, default="
__MODULE_STRING(DEFAULT_TIMEOUT) ")");
Why _param in the module parameter names ? Seems to be redundant.
+static unsigned int max_timeout_param = UINT_MAX; +module_param(max_timeout_param, uint, 0); +MODULE_PARM_DESC(max_timeout_param,
"Watchdog max timeout in seconds. (>=0, default="
__MODULE_STRING(UINT_MAX) ")");
Why do we want or need this parameter and max_pretimeout ? Those are determined by the hardware.
+static unsigned int pretimeout_param; +module_param(pretimeout_param, uint, 0); +MODULE_PARM_DESC(pretimeout_param,
"Watchdog pretimeout in seconds. (>=0, default="
__MODULE_STRING(DEFAULT_PRETIMEOUT) ")");
+static unsigned int max_pretimeout_param = U32_MAX; +module_param(max_pretimeout_param, uint, 0); +MODULE_PARM_DESC(max_pretimeout_param,
"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) ")");
+/*
- help functions for accessing 32bit registers of SBSA Generic Watchdog
- */
+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 functions 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 update the 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);
+}
After understanding the watchdog a bit better now, I think you should drop those updates and set min_timeout = 1 max_timeout = min(UINT_MAX, U64_MAX / clk) min_pretimeout = 0 max_pretimeout = U32_MAX / clk
and then ensure that pretimeout < timeout at runtime (if possible in the infrastructure code).
Even at maximum clock rate, max_timeout is so large that anything else is really overkill.
+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;
So in practice you always have a pretimeout of at least one second, correct ? That kind of violates the ABI, which says that the pretimeout should be disabled if set to 0. Is there anything we can do about that ?
What exactly happens if WOR = 0 ? Doesn't that just mean that the second timeout will happen immediately, and isn't that what we would want if pretimeout = 0 ?
- /* refresh the WOR, that will cause an explicit watchdog refresh */
Except that we use wcv which needs a manual refresh, so this is a bit misleading, isn't it ?
- 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);
Should that happen after writing wcv ?
- 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;
- /*
* Get the frequency of system counter from
* the cp15 interface of ARM Generic timer
*/
- clk = arch_timer_get_cntfrq();
That can not return 0, presumably, from looking into its implementation. And the system should panic if it is ever 0.
- if (!clk) {
Given that, I don't think this check is necessary.
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(cf_base))
return PTR_ERR(cf_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));
Does this message (specifically the WCS / WCV values) have any useful meaning for the user ?
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);
You have not configured wdd->timeout and wdd->pretimeout here, yet the function calls reload_timeout_to_wcv which needs it.
- }
- wdd->min_pretimeout = 0;
- wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout_param);
- wdd->min_timeout = 1;
- do_div(first_period_max, clk);
- wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),
max_timeout_param);
- wdd->pretimeout = DEFAULT_PRETIMEOUT;
- wdd->timeout = DEFAULT_TIMEOUT;
- watchdog_init_timeouts(wdd, pretimeout_param, timeout_param, dev);
- /* update pretimeout to WOR */
- sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd);
This is interesting because you set WOR to 0 here if pretimeout is 0. Yet, when pretimeout is updated later on, you always set it to at least one second. Any reason for doing this differently ?
If the above works, and presumably you have tested it, I don't see why it would be necessary to set WOR to a value larger than 0 when updating pretimeout.
- 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);
400000Hz reads a bit odd. How about a space between the number and Hz ?
- 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);
I don't think you should do anything here. The driver can only be removed if closed, and it that case the watchdog will already have been stopped, or if nowayout was set it won't be stopped. Either case it is already in the state we want it to be in.
- 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, Great thanks, feedback inline
On 26 May 2015 at 03:39, Guenter Roeck linux@roeck-us.net wrote:
On 05/25/2015 03:03 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, the interrupt routine run panic to save system context.
Acked-by: Arnd Bergmann arnd@arndb.de Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Signed-off-by: Fu Wei fu.wei@linaro.org
Comments inline.
Thanks, Guenter
drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/sbsa_gwdt.c | 474 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 486 insertions(+) create mode 100644 drivers/watchdog/sbsa_gwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index e5e7c55..554f18a 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -152,6 +152,17 @@ config ARM_SP805_WATCHDOG ARM Primecell SP805 Watchdog timer. This will reboot your system when the timeout is reached.
+config ARM_SBSA_WATCHDOG
tristate "ARM SBSA Generic Watchdog"
depends on ARM64
depends on ARM_ARCH_TIMER
select WATCHDOG_CORE
help
ARM SBSA Generic Watchdog. This watchdog has two Watchdog
timeouts.
The first timeout will trigger a panic; the second timeout will
trigger a system reset.
More details: ARM 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..0d1aff1 --- /dev/null +++ b/drivers/watchdog/sbsa_gwdt.c @@ -0,0 +1,474 @@ +/*
- 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: ARM DEN0029B - Server Base System Architecture
(SBSA)
- Kernel/API: P---------| pretimeout
|-------------------------------T timeout
- SBSA GWDT: P--WOR---WS1 pretimeout
|-------WCV----------WS0~~~~~~~~T timeout
- */
+#include <linux/io.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/uaccess.h> +#include <linux/watchdog.h> +#include <asm/arch_timer.h>
+/* SBSA Generic Watchdog register definitions */ +/* refresh frame */ +#define SBSA_GWDT_WRR 0x000
+/* control frame */ +#define SBSA_GWDT_WCS 0x000 +#define SBSA_GWDT_WOR 0x008 +#define SBSA_GWDT_WCV_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*/
That is a bit low for a default. Is that on purpose ? Most drivers use 30 or 60 seconds.
That is not on purpose. will fixed them : #define DEFAULT_TIMEOUT 30 /* seconds, the 1st + 2nd watch periods*/ #define DEFAULT_PRETIMEOUT 10 /* seconds, the 2nd watch period*/
+#define DEFAULT_PRETIMEOUT 5 /* seconds, the 2nd watch period*/
+static unsigned int timeout_param; +module_param(timeout_param, uint, 0); +MODULE_PARM_DESC(timeout_param,
"Watchdog timeout in seconds. (>=0, default="
__MODULE_STRING(DEFAULT_TIMEOUT) ")");
Why _param in the module parameter names ? Seems to be redundant.
sorry, I shouldn't add _param , fixed it
+static unsigned int max_timeout_param = UINT_MAX; +module_param(max_timeout_param, uint, 0); +MODULE_PARM_DESC(max_timeout_param,
"Watchdog max timeout in seconds. (>=0, default="
__MODULE_STRING(UINT_MAX) ")");
Why do we want or need this parameter and max_pretimeout ? Those are determined by the hardware.
Sorry, I thought we can also allow user setup a max value when insmod the module or by boot cmdline. but I was wrong, have deleted them.
+static unsigned int pretimeout_param; +module_param(pretimeout_param, uint, 0); +MODULE_PARM_DESC(pretimeout_param,
"Watchdog pretimeout in seconds. (>=0, default="
__MODULE_STRING(DEFAULT_PRETIMEOUT) ")");
+static unsigned int max_pretimeout_param = U32_MAX; +module_param(max_pretimeout_param, uint, 0); +MODULE_PARM_DESC(max_pretimeout_param,
"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) ")");
+/*
- help functions for accessing 32bit registers of SBSA Generic Watchdog
- */
+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 functions 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 update the 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);
+}
After understanding the watchdog a bit better now, I think you should drop those updates and set min_timeout = 1 max_timeout = min(UINT_MAX, U64_MAX / clk) min_pretimeout = 0 max_pretimeout = U32_MAX / clk
and then ensure that pretimeout < timeout at runtime (if possible in the infrastructure code).
Even at maximum clock rate, max_timeout is so large that anything else is really overkill.
you are right, fixed it, thanks
+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;
So in practice you always have a pretimeout of at least one second, correct ? That kind of violates the ABI, which says that the pretimeout should be disabled if set to 0. Is there anything we can do about that ?
What exactly happens if WOR = 0 ? Doesn't that just mean that the second timeout will happen immediately, and isn't that what we would want if pretimeout = 0
Accroding to SBSA, in theory, if WOR == 0, once we write WRR(refresh), we will get WS0 and WS1 immediately . that means timeout == 0.
So I will add more comment here to explain this.
/* refresh the WOR, that will cause an explicit watchdog refresh
*/
Except that we use wcv which needs a manual refresh, so this is a bit misleading, isn't it ?
if we setup wcv manually, we maybe don't need a manual refresh. I will test it later.
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);
Should that happen after writing wcv ?
no, if so, WOR + current_time will be loaded into wcv. I put this before writing wcv on purpose. maybe we can just update wcv without a force refresh.
will test it later
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;
/*
* Get the frequency of system counter from
* the cp15 interface of ARM Generic timer
*/
clk = arch_timer_get_cntfrq();
That can not return 0, presumably, from looking into its implementation. And the system should panic if it is ever 0.
if (!clk) {
Given that, I don't think this check is necessary.
yes, you are right , I have got your point, and improved it .
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(cf_base))
return PTR_ERR(cf_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));
Does this message (specifically the WCS / WCV values) have any useful meaning for the user ?
I think so, according to SBSA spec: If WS0 is asserted and a timeout refresh occurs then the following must occur: If the system is compliant to SBSA level 2 or higher the compare value must retain its current value. This means that the compare value records the time that WS1 is asserted.
So, I think WCV can log the time when system reset by WDT, that may help user figure out the problem. but WCS can be delete I think.
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);
You have not configured wdd->timeout and wdd->pretimeout here, yet the function calls reload_timeout_to_wcv which needs it.
Ah, sorry, that is a bug, have reordered the code, Thanks
}
wdd->min_pretimeout = 0;
wdd->max_pretimeout = min(U32_MAX / clk, max_pretimeout_param);
wdd->min_timeout = 1;
do_div(first_period_max, clk);
wdd->max_timeout = min((uint)(wdd->pretimeout + first_period_max),
max_timeout_param);
wdd->pretimeout = DEFAULT_PRETIMEOUT;
wdd->timeout = DEFAULT_TIMEOUT;
watchdog_init_timeouts(wdd, pretimeout_param, timeout_param, dev);
/* update pretimeout to WOR */
sbsa_gwdt_cf_write(SBSA_GWDT_WOR, wdd->pretimeout * clk, wdd);
This is interesting because you set WOR to 0 here if pretimeout is 0. Yet, when pretimeout is updated later on, you always set it to at least one second. Any reason for doing this differently ?
If the above works, and presumably you have tested it, I don't see why it would be necessary to set WOR to a value larger than 0 when updating pretimeout.
Ah I should tested more in extreme case, but here, I can use sbsa_gwdt_set_pretimeout directly.
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);
400000Hz reads a bit odd. How about a space between the number and Hz ?
np, fixed it , thanks :-)
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);
I don't think you should do anything here. The driver can only be removed if closed, and it that case the watchdog will already have been stopped, or if nowayout was set it won't be stopped. Either case it is already in the state we want it to be in.
Sorry, I should delete it. Fixed
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/29/2015 02:11 AM, Fu Wei wrote: [ ... ]
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));
Does this message (specifically the WCS / WCV values) have any useful meaning for the user ?
I think so, according to SBSA spec: If WS0 is asserted and a timeout refresh occurs then the following must occur: If the system is compliant to SBSA level 2 or higher the compare value must retain its current value. This means that the compare value records the time that WS1 is asserted.
So, I think WCV can log the time when system reset by WDT, that may help user figure out the problem. but WCS can be delete I think.
How would that help ? It is just a number with no reference to anything.
Thanks, Guenter
Hi Guenter,
On 29 May 2015 at 22:54, Guenter Roeck linux@roeck-us.net wrote:
On 05/29/2015 02:11 AM, Fu Wei wrote: [ ... ]
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));
Does this message (specifically the WCS / WCV values) have any useful meaning for the user ?
I think so, according to SBSA spec: If WS0 is asserted and a timeout refresh occurs then the following must occur: If the system is compliant to SBSA level 2 or higher the compare value must retain its current value. This means that the compare value records the time that WS1 is asserted.
So, I think WCV can log the time when system reset by WDT, that may help user figure out the problem. but WCS can be delete I think.
How would that help ? It is just a number with no reference to anything.
This number is the time, when system reset by watchdog WCV : the number of clock, from system boot to system reset. Is that worthy to be logged? any suggestion ? :-)
Thanks, Guenter
On 05/25/2015 05:03 AM, fu.wei@linaro.org wrote:
+/*
- help functions for accessing 32bit registers of SBSA Generic Watchdog
- */
+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);
+}
I don't understand the value of these functions. You're just adding overhead to each read and write by dereferencing wdd every time. I would get rid of them and just call readl_relaxed() and writel_relaxed() directly.
+/*
- help functions 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));
Please add a comment indicating that you're trying to read WCV atomically.
- return (((u64)wcv_hi << 32) | wcv_lo);
+}
How about defining this macro:
#define make64(high, low) (((u64)(high) << 32) | (low))
and using it instead? That makes the code easier to read.
+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;
Use upper_32_bits() and lower_32_bits() instead.
- 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)
This should be sbsa_gwdt_reload_timeout_to_wcv()
+{
- 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);
Shouldn't you program WCV and WOR together?
+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);
Why not just ping the watchdog explicitely?
+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)
This should always be true. Instead of reading WCS, I think you should just panic().
+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;
- /*
* Get the frequency of system counter from
* the cp15 interface of ARM Generic timer
*/
- clk = arch_timer_get_cntfrq();
- if (!clk) {
You have
depends on ARM_ARCH_TIMER
in your Kconfig, so you don't need to check the return of arch_timer_get_cntfrq(). It can never be zero.
Also, I would not use the variable name 'clk', because that's usually used for a "struct clk" object. I would call this "freq" instead.
Hi Timur,
On 27 May 2015 at 00:50, Timur Tabi timur@codeaurora.org wrote:
On 05/25/2015 05:03 AM, fu.wei@linaro.org wrote:
+/*
- help functions for accessing 32bit registers of SBSA Generic Watchdog
- */
+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);
+}
I don't understand the value of these functions. You're just adding overhead to each read and write by dereferencing wdd every time. I would get rid of them and just call readl_relaxed() and writel_relaxed() directly.
yes, that makes sense, sometimes , I also feel these functions are a little redundant, let me see if I can improve it.
+/*
- help functions 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));
Please add a comment indicating that you're trying to read WCV atomically.
OK , that makes sense
return (((u64)wcv_hi << 32) | wcv_lo);
+}
How about defining this macro:
#define make64(high, low) (((u64)(high) << 32) | (low))
and using it instead? That makes the code easier to read.
good idea, but it's just used once, not sure if it's worthy Actually, I have seen some macro in some driver, but not in kernel header file.
+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;
Use upper_32_bits() and lower_32_bits() instead.
cool, thanks , fixed it
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)
This should be sbsa_gwdt_reload_timeout_to_wcv()
+{
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);
Shouldn't you program WCV and WOR together?
why? WOR just for pretimeout in this driver.
+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);
Why not just ping the watchdog explicitely?
we just setup WOR, but we don't need to load pretimeout to WCV now, right ?
+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)
This should always be true. Instead of reading WCS, I think you should just panic().
I thinks I need to confirm it , in case this has been cleaned.
+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;
/*
* Get the frequency of system counter from
* the cp15 interface of ARM Generic timer
*/
clk = arch_timer_get_cntfrq();
if (!clk) {
You have
depends on ARM_ARCH_TIMER
in your Kconfig, so you don't need to check the return of arch_timer_get_cntfrq(). It can never be zero.
Also, I would not use the variable name 'clk', because that's usually used for a "struct clk" object. I would call this "freq" instead.
yes, I have fixed it .
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Fu Wei wrote:
This should always be true. Instead of reading WCS, I think you should just
panic().
I thinks I need to confirm it , in case this has been cleaned.
I don't see how it's possible for you to receive the interrupt and have WCS be cleared.
Hi Timur
On 29 May 2015 at 21:28, Timur Tabi timur@codeaurora.org wrote:
Fu Wei wrote:
This should always be true. Instead of reading WCS, I think you should just
panic().
I thinks I need to confirm it , in case this has been cleaned.
I don't see how it's possible for you to receive the interrupt and have WCS be cleared.
It is a SPI, every CPU can get it, But maybe I miss something, but please let me know if other CPU can not get the interrupt.
-- 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/29/2015 09:32 AM, Fu Wei wrote:
It is a SPI, every CPU can get it, But maybe I miss something, but please let me know if other CPU can not get the interrupt.
There's only one watchdog device, so there's only one interrupt. I don't know which CPU will get the interrupt, but the watchdog is not a per-CPU device.
Hi Timur
On 29 May 2015 at 23:46, Timur Tabi timur@codeaurora.org wrote:
On 05/29/2015 09:32 AM, Fu Wei wrote:
It is a SPI, every CPU can get it, But maybe I miss something, but please let me know if other CPU can not get the interrupt.
There's only one watchdog device, so there's only one interrupt. I don't know which CPU will get the interrupt, but the watchdog is not a per-CPU device.
OK, first of all, I know, there maybe only watchdog in the system, but the Interrupter is SPI, that means maybe some other device can also trigger the same Interrupter or maybe(in SMP) other CPU will handle it.
If this interrupter is triggered, that means system has goes wrong, who knows what is wrong , I have to make sure that system get into that routine ,because of the WS0, if not I won't do panic.
And in a interrupter routine , checking the Interrupter status register is right way to do.
-- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On 05/29/2015 12:53 PM, Fu Wei wrote:
If this interrupter is triggered, that means system has goes wrong, who knows what is wrong , I have to make sure that system get into that routine ,because of the WS0, if not I won't do panic.
But the interrupt handler is not registered as shared, which means that it cannot be generated by another device.
And in a interrupter routine , checking the Interrupter status register is right way to do.
If you get an interrupt, but WS0 is not set, then you should return IRQ_NONE instead of IRQ_HANDLED.
Also, I don't think IRQF_TIMER is correct. It's not a timer interrupt. Watchdogs are *not* timers.
On 05/29/2015 08:46 AM, Timur Tabi wrote:
On 05/29/2015 09:32 AM, Fu Wei wrote:
It is a SPI, every CPU can get it, But maybe I miss something, but please let me know if other CPU can not get the interrupt.
There's only one watchdog device, so there's only one interrupt. I don't know which CPU will get the interrupt, but the watchdog is not a per-CPU device.
Plus, that one interrupt is not shared, and the driver returns IRQ_HANDLED even if the bit is not set. So _something_ is definitely wrong. Either the interrupt is shared, then it needs to be requested as shared and the handler should only return IRQ_HANDLED if it actually handles the interrupt. Or it is not shared and the handler should always handle it.
Guenter
Hi Guenter , Timur
On 30 May 2015 at 06:10, Guenter Roeck linux@roeck-us.net wrote:
On 05/29/2015 08:46 AM, Timur Tabi wrote:
On 05/29/2015 09:32 AM, Fu Wei wrote:
It is a SPI, every CPU can get it, But maybe I miss something, but please let me know if other CPU can not get the interrupt.
There's only one watchdog device, so there's only one interrupt. I don't know which CPU will get the interrupt, but the watchdog is not a per-CPU device.
Plus, that one interrupt is not shared, and the driver returns IRQ_HANDLED even if the bit is not set. So _something_ is definitely wrong. Either the interrupt is shared, then it needs to be requested as shared and the handler should only return IRQ_HANDLED if it actually handles the interrupt. Or it is not shared and the handler should always handle it.
I have thought about this again, For now, I did not find any reason to keep that "if (status & SBSA_GWDT_WCS_WS0)"
So you are right, I should delete it.
and for IRQF_TIMER, I will delete it.
Thanks for your correction.
Guenter
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.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Fu Wei fu.wei@linaro.org --- arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..c95deec 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,147 @@ 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_debug("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; + } + + /* + * Add a platform device named "sbsa-gwdt" to match the platform driver. + * "sbsa-gwdt": SBSA(Server Base System Architecture) Generic Watchdog + * The platform driver can get device info below by matching this name. + */ + 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; + + /* + * According to SBSA specification the size of refresh and control + * frames of SBSA Generic Watchdog is SZ_4K(Offset 0x000 – 0xFFF). + */ + 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_warn("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);
Hi Fu Wei,
Some minor comments inline.
On 2015年05月25日 18:03, 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.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8b83955..c95deec 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,147 @@ void __init acpi_gic_init(void)
early_acpi_os_unmap_memory((char *)table, tbl_size); }
please add
#ifdef CONFIG_ARM_SBSA_WATCHDOG (acpi gtdt code) #endif
+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_debug("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;
- }
- /*
* Add a platform device named "sbsa-gwdt" to match the platform driver.
* "sbsa-gwdt": SBSA(Server Base System Architecture) Generic Watchdog
* The platform driver can get device info below by matching this name.
* The platform driver (drivers/watchdog/sbsa_gwdt.c) can get device info below by matching this name.
Adding the file name which will help for review and maintain in my opinion.
*/
- 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;
- /*
* According to SBSA specification the size of refresh and control
* frames of SBSA Generic Watchdog is SZ_4K(Offset 0x000 – 0xFFF).
*/
- 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;
How about
if (!err) return 0;
then no need for goto err_free_res and save two lines of codes.
Other than that,
Acked-by: Hanjun Guo hanjun.guo@linaro.org
Thanks Hanjun
On 05/26/2015 03:28 AM, Hanjun Guo wrote:
early_acpi_os_unmap_memory((char *)table, tbl_size);
}
please add
#ifdef CONFIG_ARM_SBSA_WATCHDOG (acpi gtdt code) #endif
I don't agree with this. The GTDT should be parsed even if there's no watchdog driver compiled for this kernel. There are no other #ifdefs in this file.
* Add a platform device named "sbsa-gwdt" to match the platform
driver.
* "sbsa-gwdt": SBSA(Server Base System Architecture) Generic
Watchdog
* The platform driver can get device info below by matching this
name.
- The platform driver (drivers/watchdog/sbsa_gwdt.c) can get device info
below by matching this name.
Adding the file name which will help for review and maintain in my opinion.
Except it will cause problems if the driver is renamed or moved. I don't think this is a good idea, either (sorry!)
On Tue, May 26, 2015 at 11:35:19AM -0500, Timur Tabi wrote:
On 05/26/2015 03:28 AM, Hanjun Guo wrote:
early_acpi_os_unmap_memory((char *)table, tbl_size);
}
please add
#ifdef CONFIG_ARM_SBSA_WATCHDOG (acpi gtdt code) #endif
I don't agree with this. The GTDT should be parsed even if there's no watchdog driver compiled for this kernel. There are no other #ifdefs in this file.
Me not either, but then I thought this is really a maintainer question. If the maintainers want out-of-subsystem #ifdefs in the code, who am I to complain.
Should be "#if IS_ENABLED(CONFIG_ARM_SBSA_WATCHDOG)", though, if we go along that route.
Guenter
On 2015年05月27日 00:35, Timur Tabi wrote:
On 05/26/2015 03:28 AM, Hanjun Guo wrote:
early_acpi_os_unmap_memory((char *)table, tbl_size);
}
please add
#ifdef CONFIG_ARM_SBSA_WATCHDOG (acpi gtdt code) #endif
I don't agree with this. The GTDT should be parsed even if there's no watchdog driver compiled for this kernel. There are no other #ifdefs in this file.
So what's the point of parse GTDT and alloc memories for it if there is no watchdog driver compiled for the kernel? will the module insmod later even if the CONFIG_ARM_SBSA_WATCHDOG=n?
* Add a platform device named "sbsa-gwdt" to match the platform
driver.
* "sbsa-gwdt": SBSA(Server Base System Architecture) Generic
Watchdog
* The platform driver can get device info below by matching this
name.
- The platform driver (drivers/watchdog/sbsa_gwdt.c) can get device info
below by matching this name.
Adding the file name which will help for review and maintain in my opinion.
Except it will cause problems if the driver is renamed or moved. I don't think this is a good idea, either (sorry!)
OK, that's good point. but what I proposed is some hint to which driver will use the data prepared in this file, we can easily understand it in this patchset, but if just review the code in this fiel, I think people will be confused without detail comments.
Thanks Hanjun
Hanjun Guo wrote:
I don't agree with this. The GTDT should be parsed even if there's no watchdog driver compiled for this kernel. There are no other #ifdefs in this file.
So what's the point of parse GTDT and alloc memories for it if there is no watchdog driver compiled for the kernel?
I don't think it's normal policy to generate a platform only if one specific driver is enabled.
will the module insmod later even if the CONFIG_ARM_SBSA_WATCHDOG=n?
I think that actually can work, but it's not a good reason by itself.
OK, that's good point. but what I proposed is some hint to which driver will use the data prepared in this file, we can easily understand it in this patchset, but if just review the code in this fiel, I think people will be confused without detail comments.
All anyone needs to is
git grep "sbsa-gwdt"
And you'll find the driver.
On Mon, May 25, 2015 at 11:03:13AM +0100, 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.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)
Why does this all need to be under arch/arm64? The GTDT really isn't architecture-specific, so I'd *much* rather it was parsed in the driver code itself, like we already do for the architected timer. The GIC is an exception because it's in the MADT, which we need to parse in the arch code to configure SMP properly.
Will
Hi Will,
On 26 May 2015 at 08:28, Will Deacon will.deacon@arm.com wrote:
On Mon, May 25, 2015 at 11:03:13AM +0100, 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.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)
Why does this all need to be under arch/arm64? The GTDT really isn't architecture-specific, so I'd *much* rather it was parsed in the driver code itself, like we already do for the architected timer. The GIC is an exception because it's in the MADT, which we need to parse in the arch code to configure SMP properly.
I'm not really against refactoring the code. But the GTDT looks quite specific to ARM..
---8<---- 5.2.24 Generic Timer Description Table (GTDT) This section describes the format of the Generic Timer Description Table (GTDT), which provides OSPM with information about a system’s Generic Timers configuration. The Generic Timer (GT) is a standard timer interface implemented on ARM processor-based systems. The GT hardware specification can be found at Links to ACPI-Related Documents (http://uefi.org/acpi) under the heading ARM Architecture. The GTDT provides OSPM with information about a system's GT interrupt configurations, for both per-processor timers, and platform (memory-mapped) timers. The GT specification defines the following per-processor timers: • Secure privilege level 1 (EL1) timer, • Non-Secure EL1 timer, • Non-Secure privilege level 2 (EL2) timer, • Virtual timer, and the following Platform (memory-mapped) timers. • GT Block • Server Base System Architecture (SBSA) Generic Watchdog ---8<----
Thanks, Ashwin.
On Tue, May 26, 2015 at 04:02:56PM +0100, Ashwin Chaugule wrote:
On 26 May 2015 at 08:28, Will Deacon will.deacon@arm.com wrote:
On Mon, May 25, 2015 at 11:03:13AM +0100, 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.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)
Why does this all need to be under arch/arm64? The GTDT really isn't architecture-specific, so I'd *much* rather it was parsed in the driver code itself, like we already do for the architected timer. The GIC is an exception because it's in the MADT, which we need to parse in the arch code to configure SMP properly.
I'm not really against refactoring the code. But the GTDT looks quite specific to ARM..
---8<---- 5.2.24 Generic Timer Description Table (GTDT) This section describes the format of the Generic Timer Description Table (GTDT), which provides OSPM with information about a system’s Generic Timers configuration. The Generic Timer (GT) is a standard timer interface implemented on ARM processor-based systems. The GT hardware specification can be found at Links to ACPI-Related Documents (http://uefi.org/acpi) under the heading ARM Architecture. The GTDT provides OSPM with information about a system's GT interrupt configurations, for both per-processor timers, and platform (memory-mapped) timers. The GT specification defines the following per-processor timers: • Secure privilege level 1 (EL1) timer, • Non-Secure EL1 timer, • Non-Secure privilege level 2 (EL2) timer, • Virtual timer, and the following Platform (memory-mapped) timers. • GT Block • Server Base System Architecture (SBSA) Generic Watchdog ---8<----
Sure, the device it describes may only ever exist on ARM systems, but by that logic then we should be moving lots of drivers back under arch/arm[64].
The ARM architecture says precisely *nothing* about ACPI, so we should try to keep arch/arm64/kernel/acpi.c to a minimum and not shovel all sorts of table conversion code in there for random peripherals.
Will
On 26 May 2015 at 11:18, Will Deacon will.deacon@arm.com wrote:
On Tue, May 26, 2015 at 04:02:56PM +0100, Ashwin Chaugule wrote:
On 26 May 2015 at 08:28, Will Deacon will.deacon@arm.com wrote:
On Mon, May 25, 2015 at 11:03:13AM +0100, 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.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)
Why does this all need to be under arch/arm64? The GTDT really isn't architecture-specific, so I'd *much* rather it was parsed in the driver code itself, like we already do for the architected timer. The GIC is an exception because it's in the MADT, which we need to parse in the arch code to configure SMP properly.
I'm not really against refactoring the code. But the GTDT looks quite specific to ARM..
---8<---- 5.2.24 Generic Timer Description Table (GTDT) This section describes the format of the Generic Timer Description Table (GTDT), which provides OSPM with information about a system’s Generic Timers configuration. The Generic Timer (GT) is a standard timer interface implemented on ARM processor-based systems. The GT hardware specification can be found at Links to ACPI-Related Documents (http://uefi.org/acpi) under the heading ARM Architecture. The GTDT provides OSPM with information about a system's GT interrupt configurations, for both per-processor timers, and platform (memory-mapped) timers. The GT specification defines the following per-processor timers: • Secure privilege level 1 (EL1) timer, • Non-Secure EL1 timer, • Non-Secure privilege level 2 (EL2) timer, • Virtual timer, and the following Platform (memory-mapped) timers. • GT Block • Server Base System Architecture (SBSA) Generic Watchdog ---8<----
Sure, the device it describes may only ever exist on ARM systems, but by that logic then we should be moving lots of drivers back under arch/arm[64].
Sure. Not arguing about that. :) You said the GTDT isn't really arch specific. That was a bit confusing.
The ARM architecture says precisely *nothing* about ACPI, so we should try to keep arch/arm64/kernel/acpi.c to a minimum and not shovel all sorts of table conversion code in there for random peripherals.
Will
On Tue, May 26, 2015 at 04:18:42PM +0100, Will Deacon wrote:
On Tue, May 26, 2015 at 04:02:56PM +0100, Ashwin Chaugule wrote:
On 26 May 2015 at 08:28, Will Deacon will.deacon@arm.com wrote:
On Mon, May 25, 2015 at 11:03:13AM +0100, 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.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)
Why does this all need to be under arch/arm64? The GTDT really isn't architecture-specific, so I'd *much* rather it was parsed in the driver code itself, like we already do for the architected timer. The GIC is an exception because it's in the MADT, which we need to parse in the arch code to configure SMP properly.
I'm not really against refactoring the code. But the GTDT looks quite specific to ARM..
---8<---- 5.2.24 Generic Timer Description Table (GTDT) This section describes the format of the Generic Timer Description Table (GTDT), which provides OSPM with information about a system’s Generic Timers configuration. The Generic Timer (GT) is a standard timer interface implemented on ARM processor-based systems. The GT hardware specification can be found at Links to ACPI-Related Documents (http://uefi.org/acpi) under the heading ARM Architecture. The GTDT provides OSPM with information about a system's GT interrupt configurations, for both per-processor timers, and platform (memory-mapped) timers. The GT specification defines the following per-processor timers: • Secure privilege level 1 (EL1) timer, • Non-Secure EL1 timer, • Non-Secure privilege level 2 (EL2) timer, • Virtual timer, and the following Platform (memory-mapped) timers. • GT Block • Server Base System Architecture (SBSA) Generic Watchdog ---8<----
Sure, the device it describes may only ever exist on ARM systems, but by that logic then we should be moving lots of drivers back under arch/arm[64].
It is nt the driver, but its instantiation. The question here would be how and where to instantiate the driver, not where the driver itself is located. The driver itself is ACPI agnostic.
What you are really saying is that you want the driver instantiation to be moved into the driver.
Guenter
On 26 May 2015 at 23:36, Guenter Roeck linux@roeck-us.net wrote:
On Tue, May 26, 2015 at 04:18:42PM +0100, Will Deacon wrote:
On Tue, May 26, 2015 at 04:02:56PM +0100, Ashwin Chaugule wrote:
On 26 May 2015 at 08:28, Will Deacon will.deacon@arm.com wrote:
On Mon, May 25, 2015 at 11:03:13AM +0100, 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.
Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Timur Tabi timur@codeaurora.org Signed-off-by: Fu Wei fu.wei@linaro.org
arch/arm64/kernel/acpi.c | 145 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+)
Why does this all need to be under arch/arm64? The GTDT really isn't architecture-specific, so I'd *much* rather it was parsed in the driver code itself, like we already do for the architected timer. The GIC is an exception because it's in the MADT, which we need to parse in the arch code to configure SMP properly.
I'm not really against refactoring the code. But the GTDT looks quite specific to ARM..
---8<---- 5.2.24 Generic Timer Description Table (GTDT) This section describes the format of the Generic Timer Description Table (GTDT), which provides OSPM with information about a system’s Generic Timers configuration. The Generic Timer (GT) is a standard timer interface implemented on ARM processor-based systems. The GT hardware specification can be found at Links to ACPI-Related Documents (http://uefi.org/acpi) under the heading ARM Architecture. The GTDT provides OSPM with information about a system's GT interrupt configurations, for both per-processor timers, and platform (memory-mapped) timers. The GT specification defines the following per-processor timers: • Secure privilege level 1 (EL1) timer, • Non-Secure EL1 timer, • Non-Secure privilege level 2 (EL2) timer, • Virtual timer, and the following Platform (memory-mapped) timers. • GT Block • Server Base System Architecture (SBSA) Generic Watchdog ---8<----
Sure, the device it describes may only ever exist on ARM systems, but by that logic then we should be moving lots of drivers back under arch/arm[64].
It is nt the driver, but its instantiation. The question here would be how and where to instantiate the driver, not where the driver itself is located. The driver itself is ACPI agnostic.
Hi Will,
I really don't mind to refactor the code, If we can make this patch better.
But for now, I can't see the good reason to move ACPI-relevant code into a watchdog driver.
The reasons I put the code here are (1)SBSA watchdog only for ARM64 (2)GTDT only for ARM, design for ARM, (3)For ARM Architecture, only ARM64 support ACPI.
For minimizing arch/arm64/kernel/acpi.c, we can't put the code here, and we had better keep these code outside the driver,
So do you have any suggestion for the better location of the GTDT code?
Great thanks for your help.
What you are really saying is that you want the driver instantiation to be moved into the driver.
Guenter
On Tue, May 26, 2015 at 05:27:33PM +0100, Fu Wei wrote:
On 26 May 2015 at 23:36, Guenter Roeck linux@roeck-us.net wrote:
On Tue, May 26, 2015 at 04:18:42PM +0100, Will Deacon wrote:
Sure, the device it describes may only ever exist on ARM systems, but by that logic then we should be moving lots of drivers back under arch/arm[64].
It is nt the driver, but its instantiation. The question here would be how and where to instantiate the driver, not where the driver itself is located. The driver itself is ACPI agnostic.
I really don't mind to refactor the code, If we can make this patch better.
But for now, I can't see the good reason to move ACPI-relevant code into a watchdog driver.
I don't really mind where you move it, just as long as it's outside of arch/arm64.
The reasons I put the code here are (1)SBSA watchdog only for ARM64 (2)GTDT only for ARM, design for ARM, (3)For ARM Architecture, only ARM64 support ACPI.
For minimizing arch/arm64/kernel/acpi.c, we can't put the code here, and we had better keep these code outside the driver,
So do you have any suggestion for the better location of the GTDT code?
I don't understand why you can't do the same as drivers/clocksource/arm_arch_timer.c and parse the table directly in the driver. If there are objections from the driver/subsystem maintainers then it sounds like we need a mechanical ACPI table -> platform device conversion in the core, like we have for device-tree.
Will
Hi Will,
As you know, I have moved all the GTDT code to ACPI driver , and simplify the GTDT relevant code in arm_arch_timer.c. That will be in my next patchset. but you can check here : https://git.linaro.org/people/fu.wei/linux.git/shortlog/refs/heads/acpi-topi...
Great thanks for your suggestion!
On 27 May 2015 at 18:44, Will Deacon will.deacon@arm.com wrote:
On Tue, May 26, 2015 at 05:27:33PM +0100, Fu Wei wrote:
On 26 May 2015 at 23:36, Guenter Roeck linux@roeck-us.net wrote:
On Tue, May 26, 2015 at 04:18:42PM +0100, Will Deacon wrote:
Sure, the device it describes may only ever exist on ARM systems, but by that logic then we should be moving lots of drivers back under arch/arm[64].
It is nt the driver, but its instantiation. The question here would be how and where to instantiate the driver, not where the driver itself is located. The driver itself is ACPI agnostic.
I really don't mind to refactor the code, If we can make this patch better.
But for now, I can't see the good reason to move ACPI-relevant code into a watchdog driver.
I don't really mind where you move it, just as long as it's outside of arch/arm64.
The reasons I put the code here are (1)SBSA watchdog only for ARM64 (2)GTDT only for ARM, design for ARM, (3)For ARM Architecture, only ARM64 support ACPI.
For minimizing arch/arm64/kernel/acpi.c, we can't put the code here, and we had better keep these code outside the driver,
So do you have any suggestion for the better location of the GTDT code?
I don't understand why you can't do the same as drivers/clocksource/arm_arch_timer.c and parse the table directly in the driver. If there are objections from the driver/subsystem maintainers then it sounds like we need a mechanical ACPI table -> platform device conversion in the core, like we have for device-tree.
Will