Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
From: Guenter Roeck
Date: Thu Dec 01 2022 - 10:56:43 EST
On 12/1/22 03:08, Michal Simek wrote:
Hi Guenter,
On 11/6/22 16:16, Neeli, Srinivas wrote:
Hi Guenter,
-----Original Message-----
From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
Sent: Thursday, November 3, 2022 10:55 PM
To: Neeli, Srinivas <srinivas.neeli@xxxxxxx>
Cc: wim@xxxxxxxxxxxxxxxxxx; Datta, Shubhrajyoti
<shubhrajyoti.datta@xxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>;
robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; linux-arm-
kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; git (AMD-Xilinx)
<git@xxxxxxx>
Subject: Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
watchdog support
On Thu, Nov 03, 2022 at 04:51:14PM +0000, Neeli, Srinivas wrote:
HI Guenter,
-----Original Message-----
From: Neeli, Srinivas <srinivas.neeli@xxxxxxx>
Sent: Tuesday, October 11, 2022 11:57 AM
To: Guenter Roeck <linux@xxxxxxxxxxxx>
Cc: wim@xxxxxxxxxxxxxxxxxx; Datta, Shubhrajyoti
<shubhrajyoti.datta@xxxxxxx>; Simek, Michal
<michal.simek@xxxxxxx>;
robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; linux-arm-
kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; git
(AMD-Xilinx) <git@xxxxxxx>
Subject: RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
watchdog support
Hi,
-----Original Message-----
From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter
Roeck
Sent: Sunday, October 2, 2022 9:55 PM
To: Neeli, Srinivas <srinivas.neeli@xxxxxxx>
Cc: wim@xxxxxxxxxxxxxxxxxx; Datta, Shubhrajyoti
<shubhrajyoti.datta@xxxxxxx>; Simek, Michal
<michal.simek@xxxxxxx>;
robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; linux-arm-
kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; git
(AMD-Xilinx) <git@xxxxxxx>
Subject: Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
watchdog support
On Tue, Sep 27, 2022 at 04:32:56PM +0530, Srinivas Neeli wrote:
Versal watchdog driver uses window watchdog mode. Window
watchdog
timer(WWDT) contains closed(first) and open(second) window with
32 bit width. Write to the watchdog timer within predefined
window periods of time. This means a period that is not too soon
and a period that is not too late. The WWDT has to be restarted
within the open window time. If software tries to restart WWDT
outside of the open window time period, it generates a reset.
Signed-off-by: Srinivas Neeli <srinivas.neeli@xxxxxxx>
---
drivers/watchdog/Kconfig | 17 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/xilinx_wwdt.c | 286
+++++++++++++++++++++++++++++++++
3 files changed, 304 insertions(+) create mode 100644
drivers/watchdog/xilinx_wwdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index
688922fc4edb..9822e471b9f0 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -304,6 +304,23 @@ config XILINX_WATCHDOG
To compile this driver as a module, choose M here: the
module will be called of_xilinx_wdt.
+config XILINX_WINDOW_WATCHDOG
+ tristate "Xilinx window watchdog timer"
+ depends on HAS_IOMEM
+ select WATCHDOG_CORE
+ help
+ Window watchdog driver for the versal_wwdt ip core.
+ Window watchdog timer(WWDT) contains closed(first) and
+ open(second) window with 32 bit width. Write to the
watchdog
+ timer within predefined window periods of time. This
means
+ a period that is not too soon and a period that is not too
+ late. The WWDT has to be restarted within the open
window time.
+ If software tries to restart WWDT outside of the open
window
+ time period, it generates a reset.
+
+ To compile this driver as a module, choose M here: the
+ module will be called xilinx_wwdt.
+
config ZIIRAVE_WATCHDOG
tristate "Zodiac RAVE Watchdog Timer"
depends on I2C
diff --git a/drivers/watchdog/Makefile
b/drivers/watchdog/Makefile index cdeb119e6e61..4ff96c517407
100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -155,6 +155,7 @@ obj-$(CONFIG_M54xx_WATCHDOG) +=
m54xx_wdt.o
# MicroBlaze Architecture
obj-$(CONFIG_XILINX_WATCHDOG) += of_xilinx_wdt.o
+obj-$(CONFIG_XILINX_WINDOW_WATCHDOG) += xilinx_wwdt.o
# MIPS Architecture
obj-$(CONFIG_ATH79_WDT) += ath79_wdt.o diff --git
a/drivers/watchdog/xilinx_wwdt.c
b/drivers/watchdog/xilinx_wwdt.c new file mode 100644 index
000000000000..2594a01c2764
--- /dev/null
+++ b/drivers/watchdog/xilinx_wwdt.c
@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Window watchdog device driver for Xilinx Versal WWDT
+ *
+ * Copyright (C) 2022, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/watchdog.h>
+
+#define XWWDT_DEFAULT_TIMEOUT 40
+#define XWWDT_MIN_TIMEOUT 1
+#define XWWDT_MAX_TIMEOUT 42
+
+/* Register offsets for the WWDT device */
+#define XWWDT_MWR_OFFSET 0x00
+#define XWWDT_ESR_OFFSET 0x04
+#define XWWDT_FCR_OFFSET 0x08
+#define XWWDT_FWR_OFFSET 0x0c
+#define XWWDT_SWR_OFFSET 0x10
+
+/* Master Write Control Register Masks */
+#define XWWDT_MWR_MASK BIT(0)
+
+/* Enable and Status Register Masks */
+#define XWWDT_ESR_WINT_MASK BIT(16)
+#define XWWDT_ESR_WSW_MASK BIT(8)
+#define XWWDT_ESR_WEN_MASK BIT(0)
+
+#define XWWDT_PERCENT 50
+
+static int xwwdt_timeout;
+static int xclosed_window_percent;
+
+module_param(xwwdt_timeout, int, 0644);
+MODULE_PARM_DESC(xwwdt_timeout,
+ "Watchdog time in seconds. (default="
+ __MODULE_STRING(XWWDT_DEFAULT_TIMEOUT)
")");
There is no reason to make this writeable. There are means to set
the timeout in runtime. Those should be used.
Accepted and will update in V2.
+module_param(xclosed_window_percent, int, 0644);
+MODULE_PARM_DESC(xclosed_window_percent,
+ "Watchdog closed window percentage. (default="
+ __MODULE_STRING(XWWDT_PERCENT) ")");
The above is problematic. This should really not be set during
runtime, and the behavior is pretty much undefined if it is
changed while the watchdog is running. It should really be set
using devicetree and not be changed in the running system.
Accepted and will update in V2.
+
+/**
+ * struct xwwdt_device - Watchdog device structure
+ * @base: base io address of WDT device
+ * @spinlock: spinlock for IO register access
+ * @xilinx_wwdt_wdd: watchdog device structure
+ * @clk: struct clk * of a clock source
+ * @freq: source clock frequency of WWDT */ struct xwwdt_device {
+ void __iomem *base;
+ spinlock_t spinlock; /* spinlock for register handling */
+ struct watchdog_device xilinx_wwdt_wdd;
+ struct clk *clk;
+ unsigned long freq;
+};
+
+static bool is_wwdt_in_closed_window(struct watchdog_device
*wdd) {
+ struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
+ u32 csr, ret;
+
+ csr = ioread32(xdev->base + XWWDT_ESR_OFFSET);
+
+ ret = (csr & XWWDT_ESR_WEN_MASK) ? !(csr &
XWWDT_ESR_WSW_MASK) ? 0 :
+1 : 1;
This is confusing.
return !(csr & XWWDT_ESR_WEN_MASK) || ((csr &
XWWDT_ESR_WSW_MASK);
should do the same and would be easier to understand, though I am
not sure if it is correct (making the point that the expression is
confusing).
Accepted and will update in V2.
+
+ return ret;
+}
+
+static int xilinx_wwdt_start(struct watchdog_device *wdd) {
+ struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
+ struct watchdog_device *xilinx_wwdt_wdd = &xdev-
xilinx_wwdt_wdd;
+ u64 time_out, closed_timeout, open_timeout;
+ u32 control_status_reg;
+
+ /* Calculate timeout count */
+ time_out = xdev->freq * wdd->timeout;
+
+ if (xclosed_window_percent) {
+ closed_timeout = (time_out *
xclosed_window_percent) /
100;
+ open_timeout = time_out - closed_timeout;
+ wdd->min_hw_heartbeat_ms =
xclosed_window_percent *
10 * wdd->timeout;
+ } else {
+ /* Calculate 50% of timeout */
Isn't that a bit random ?
Versal Window watchdog IP supports below features.
1)Start
2)Stop
3)Configure Timeout
4)Refresh
Planning to take closed window percentage from device tree parameter.
If the user hasn't passed the closed window percentage from the
device tree, by default, taking XWWDT_PERCENT value which is 50.
Does above explanation looks fine to you ?
+ time_out *= XWWDT_PERCENT;
+ time_out /= 100;
+ wdd->min_hw_heartbeat_ms = XWWDT_PERCENT *
10 *
wdd->timeout;
min_hw_heartbeat_ms is supposed to be fixed after probe. Behavior
of changing it when starting the watchdog is undefined. This will
likely fail under some conditions.
As I said in above comments versal watchdog IP supports
reconfiguration of timeout, so every restart we are updating
min_hw_heartbeat_ms based on timeout.
After stop we are reconfiguring the min_hw_heartbeat_ms, do you think still it will fail ?.
+ }
+
+ spin_lock(&xdev->spinlock);
+
+ iowrite32(XWWDT_MWR_MASK, xdev->base +
XWWDT_MWR_OFFSET);
+ iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base +
XWWDT_ESR_OFFSET);
+
+ if (xclosed_window_percent) {
+ iowrite32((u32)closed_timeout, xdev->base +
XWWDT_FWR_OFFSET);
+ iowrite32((u32)open_timeout, xdev->base +
XWWDT_SWR_OFFSET);
+ } else {
+ /* Configure closed and open windows with 50% of
timeout
*/
+ iowrite32((u32)time_out, xdev->base +
XWWDT_FWR_OFFSET);
+ iowrite32((u32)time_out, xdev->base +
XWWDT_SWR_OFFSET);
+ }
This if/else should not be necessary by using appropriate
calculations
above.
Anyway, this is moot - as said above, changing min_hw_heartbeat_ms
after probe is unexpected, and the code will have to be changed to
use a fixed value for the window size. With that, all calculations
can and should be done in the probe function.
+
+ /* Enable the window watchdog timer */
+ control_status_reg = ioread32(xdev->base +
XWWDT_ESR_OFFSET);
+ control_status_reg |= XWWDT_ESR_WEN_MASK;
+ iowrite32(control_status_reg, xdev->base +
XWWDT_ESR_OFFSET);
Why is this enabled unconditionally ? I would assume that a user
specifying a 0-percentage window size doesn't want it enabled.
Plan to add a check for closed window percentage. If user tries to
configure 100% of closed window, driver configures XWWDT_PERCENT
value.
Configuring 100% of closed window not suggestible.
Do you have any feedback on above explanation ?.
+
+ spin_unlock(&xdev->spinlock);
+
+ dev_dbg(xilinx_wwdt_wdd->parent, "Watchdog Started!\n");
+
+ return 0;
+}
+
+static int xilinx_wwdt_keepalive(struct watchdog_device *wdd) {
+ struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
+ u32 control_status_reg;
+
+ spin_lock(&xdev->spinlock);
+
+ /* Enable write access control bit for the window watchdog
*/
+ iowrite32(XWWDT_MWR_MASK, xdev->base +
XWWDT_MWR_OFFSET);
+
+ /* Trigger restart kick to watchdog */
+ control_status_reg = ioread32(xdev->base +
XWWDT_ESR_OFFSET);
+ control_status_reg |= XWWDT_ESR_WSW_MASK;
+ iowrite32(control_status_reg, xdev->base +
XWWDT_ESR_OFFSET);
+
+ spin_unlock(&xdev->spinlock);
+
+ return 0;
+}
+
+static int xilinx_wwdt_set_timeout(struct watchdog_device *wdd,
+ unsigned int new_time)
+{
+ struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
+ struct watchdog_device *xilinx_wwdt_wdd = &xdev-
xilinx_wwdt_wdd;
+
+ if (watchdog_active(xilinx_wwdt_wdd))
+ return -EPERM;
Why ? This will be the most common case and means to change the
timeout.
Versal Watchdog supports reconfiguration of timeout. If we try to
reconfigure timeout without stopping the watchdog, driver returns
error immediately. Reconfiguration of timeout, Stop and Refresh not
allowed in closed window.
User can trigger set timeout any point of time, So avoiding
reconfiguring the timeout feature using driver API if the watchdog is
active.
Please share your comments on this.
I see that there are still some pending questions on this thread.
Could you please take a look at it?
If you think that would be better to send v2 and better describe the problematic parts as the part of commit message that should be also fine.
I can only decode the comment on the bottom. I think that problem needs
a better solution. Returning -EPERM is definitely wrong here. How would
you expect userspace to react on it ? Expecting userspace to stop the
watchdog before updating the timeout is not acceptable; that is not
defined in the ABI, and we can not expect watchdog daemons to know about
it.
You could, for example:
- stop the watchdog, update the timeout, and restart it. That is
less than perfect, but other drivers with similar limitations
do it as well.
- Mark the timeout update as pending, and update it in the permitted
window (if that is possible; the above comment is vague on that).
Guenter