linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] watchdog: xilinx_wwdt: Add Versal watchdog support
@ 2022-09-27 11:02 Srinivas Neeli
  2022-09-27 11:02 ` [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device Srinivas Neeli
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Srinivas Neeli @ 2022-09-27 11:02 UTC (permalink / raw)
  To: wim, linux, srinivas.neeli, shubhrajyoti.datta, michal.simek,
	robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

This patch series does
-Adds dt-bindings for versal watchdog driver.
-Adds support for versal watchdog driver.
-Adds fragment page for xilinx watchdog drivers.

There was a series[1] sent earlier to add versal watchdog support using
pretimeout. In review it was discouraged to use pretimeout for open and
close window . This series is a new implementation of versal watchdog.

Srinivas Neeli (3):
  dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for
    xilinx window watchdog device
  watchdog: xilinx_wwdt: Add Versal window watchdog support
  MAINTAINERS: Add fragment for Xilinx watchdog drivers

 .../bindings/watchdog/xlnx,versal-wwdt.yaml   |  49 +++
 MAINTAINERS                                   |  10 +
 drivers/watchdog/Kconfig                      |  17 ++
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/xilinx_wwdt.c                | 286 ++++++++++++++++++
 5 files changed, 363 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
 create mode 100644 drivers/watchdog/xilinx_wwdt.c

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device
  2022-09-27 11:02 [PATCH 0/3] watchdog: xilinx_wwdt: Add Versal watchdog support Srinivas Neeli
@ 2022-09-27 11:02 ` Srinivas Neeli
  2022-09-29 22:20   ` Rob Herring
  2022-09-30 10:31   ` Krzysztof Kozlowski
  2022-09-27 11:02 ` [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support Srinivas Neeli
  2022-09-27 11:02 ` [PATCH 3/3] MAINTAINERS: Add fragment for Xilinx watchdog drivers Srinivas Neeli
  2 siblings, 2 replies; 17+ messages in thread
From: Srinivas Neeli @ 2022-09-27 11:02 UTC (permalink / raw)
  To: wim, linux, srinivas.neeli, shubhrajyoti.datta, michal.simek,
	robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

Add documentation for the binding of window watchdog device.

Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
---
 .../bindings/watchdog/xlnx,versal-wwdt.yaml   | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml b/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
new file mode 100644
index 000000000000..986455efa6f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/xlnx,versal-wwdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Versal window watchdog timer controller
+
+maintainers:
+  - Neeli Srinivas <srinivas.neeli@amd.com>
+
+description:
+  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.
+
+properties:
+  compatible:
+    enum:
+      - xlnx,versal-wwdt-1.0
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  timeout-sec: true
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+        watchdog@fd4d0000 {
+           compatible = "xlnx,versal-wwdt-1.0";
+           reg = <0xfd4d0000 0x10000>;
+           clocks = <&clock25>;
+           timeout-sec = <30>;
+        };
+...
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-09-27 11:02 [PATCH 0/3] watchdog: xilinx_wwdt: Add Versal watchdog support Srinivas Neeli
  2022-09-27 11:02 ` [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device Srinivas Neeli
@ 2022-09-27 11:02 ` Srinivas Neeli
  2022-09-30 10:35   ` Krzysztof Kozlowski
  2022-10-02 16:25   ` Guenter Roeck
  2022-09-27 11:02 ` [PATCH 3/3] MAINTAINERS: Add fragment for Xilinx watchdog drivers Srinivas Neeli
  2 siblings, 2 replies; 17+ messages in thread
From: Srinivas Neeli @ 2022-09-27 11:02 UTC (permalink / raw)
  To: wim, linux, srinivas.neeli, shubhrajyoti.datta, michal.simek,
	robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

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@amd.com>
---
 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) ")");
+module_param(xclosed_window_percent, int, 0644);
+MODULE_PARM_DESC(xclosed_window_percent,
+		 "Watchdog closed window percentage. (default="
+		 __MODULE_STRING(XWWDT_PERCENT) ")");
+
+/**
+ * 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;
+
+	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 */
+		time_out *= XWWDT_PERCENT;
+		time_out /= 100;
+		wdd->min_hw_heartbeat_ms = XWWDT_PERCENT * 10 * wdd->timeout;
+	}
+
+	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);
+	}
+
+	/* 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);
+
+	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;
+
+	wdd->timeout = new_time;
+
+	return 0;
+}
+
+static int xilinx_wwdt_stop(struct watchdog_device *wdd)
+{
+	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
+	struct watchdog_device *xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
+
+	if (watchdog_active(xilinx_wwdt_wdd)) {
+		if (!is_wwdt_in_closed_window(wdd)) {
+			dev_warn(xilinx_wwdt_wdd->parent, "timer in closed window");
+			return -EPERM;
+		}
+	}
+
+	spin_lock(&xdev->spinlock);
+
+	iowrite32(XWWDT_MWR_MASK, xdev->base + XWWDT_MWR_OFFSET);
+
+	/* Disable the Window watchdog timer */
+	iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base + XWWDT_ESR_OFFSET);
+
+	spin_unlock(&xdev->spinlock);
+
+	clk_disable(xdev->clk);
+
+	dev_dbg(xilinx_wwdt_wdd->parent, "Watchdog Stopped!\n");
+
+	return 0;
+}
+
+static void xwwdt_clk_disable_unprepare(void *data)
+{
+	clk_disable_unprepare(data);
+}
+
+static const struct watchdog_info xilinx_wwdt_ident = {
+	.options = WDIOF_KEEPALIVEPING |
+		WDIOF_SETTIMEOUT,
+	.firmware_version = 1,
+	.identity = "xlnx_window watchdog",
+};
+
+static const struct watchdog_ops xilinx_wwdt_ops = {
+	.owner = THIS_MODULE,
+	.start = xilinx_wwdt_start,
+	.stop = xilinx_wwdt_stop,
+	.set_timeout = xilinx_wwdt_set_timeout,
+	.ping = xilinx_wwdt_keepalive,
+};
+
+static int xwwdt_probe(struct platform_device *pdev)
+{
+	struct watchdog_device *xilinx_wwdt_wdd;
+	struct device *dev = &pdev->dev;
+	struct xwwdt_device *xdev;
+	int ret;
+
+	xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL);
+	if (!xdev)
+		return -ENOMEM;
+
+	xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
+	xilinx_wwdt_wdd->info = &xilinx_wwdt_ident;
+	xilinx_wwdt_wdd->ops = &xilinx_wwdt_ops;
+	xilinx_wwdt_wdd->parent = dev;
+
+	xdev->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(xdev->base))
+		return PTR_ERR(xdev->base);
+
+	xdev->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(xdev->clk))
+		return PTR_ERR(xdev->clk);
+
+	xdev->freq = clk_get_rate(xdev->clk);
+	if (!xdev->freq)
+		return -EINVAL;
+
+	ret = clk_prepare_enable(xdev->clk);
+	if (ret) {
+		dev_err(dev, "unable to enable clock\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, xwwdt_clk_disable_unprepare,
+				       xdev->clk);
+	if (ret)
+		return ret;
+
+	xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
+	xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
+	xilinx_wwdt_wdd->max_timeout = XWWDT_MAX_TIMEOUT;
+
+	ret = watchdog_init_timeout(xilinx_wwdt_wdd,
+				    xwwdt_timeout, &pdev->dev);
+	if (ret)
+		dev_info(&pdev->dev, "Configured default timeout value\n");
+
+	spin_lock_init(&xdev->spinlock);
+	watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
+
+	ret = devm_watchdog_register_device(dev, xilinx_wwdt_wdd);
+	if (ret)
+		return ret;
+
+	dev_info(dev, "Xilinx window watchdog Timer with timeout %ds\n",
+		 xilinx_wwdt_wdd->timeout);
+
+	return 0;
+}
+
+static const struct of_device_id xwwdt_of_match[] = {
+	{ .compatible = "xlnx,versal-wwdt-1.0", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, xwwdt_of_match);
+
+static struct platform_driver xwwdt_driver = {
+	.probe = xwwdt_probe,
+	.driver = {
+		.name = "Xilinx window watchdog",
+		.of_match_table = xwwdt_of_match,
+	},
+};
+
+module_platform_driver(xwwdt_driver);
+
+MODULE_AUTHOR("Neeli Srinivas <srinivas.neeli@amd.com>");
+MODULE_DESCRIPTION("Xilinx window watchdog driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/3] MAINTAINERS: Add fragment for Xilinx watchdog drivers
  2022-09-27 11:02 [PATCH 0/3] watchdog: xilinx_wwdt: Add Versal watchdog support Srinivas Neeli
  2022-09-27 11:02 ` [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device Srinivas Neeli
  2022-09-27 11:02 ` [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support Srinivas Neeli
@ 2022-09-27 11:02 ` Srinivas Neeli
  2 siblings, 0 replies; 17+ messages in thread
From: Srinivas Neeli @ 2022-09-27 11:02 UTC (permalink / raw)
  To: wim, linux, srinivas.neeli, shubhrajyoti.datta, michal.simek,
	robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

Added entry for Xilinx WATCHDOG drivers.

Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a34ec41fbf7a..b9771287276e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22548,6 +22548,16 @@ F:	Documentation/devicetree/bindings/media/xilinx/
 F:	drivers/media/platform/xilinx/
 F:	include/uapi/linux/xilinx-v4l2-controls.h
 
+XILINX WATCHDOG DRIVER
+M:	Srinivas Neeli <srinivas.neeli@amd.com>
+R:	Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
+R:	Michal Simek <michal.simek@amd.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
+F:	Documentation/devicetree/bindings/watchdog/xlnx,xps-timebase-wdt.yaml
+F:	drivers/watchdog/of_xilinx_wdt.c
+F:	drivers/watchdog/xilinx_wwdt.c
+
 XILINX ZYNQMP DPDMA DRIVER
 M:	Hyun Kwon <hyun.kwon@xilinx.com>
 M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device
  2022-09-27 11:02 ` [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device Srinivas Neeli
@ 2022-09-29 22:20   ` Rob Herring
  2022-09-30 10:31   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 17+ messages in thread
From: Rob Herring @ 2022-09-29 22:20 UTC (permalink / raw)
  To: Srinivas Neeli
  Cc: wim, linux, shubhrajyoti.datta, michal.simek,
	krzysztof.kozlowski+dt, linux-kernel, linux-watchdog,
	linux-arm-kernel, devicetree, git

On Tue, Sep 27, 2022 at 04:32:55PM +0530, Srinivas Neeli wrote:
> Add documentation for the binding of window watchdog device.

Subject space is precious, but you say 'watchdog' and 'binding 
documentation' (dt-bindings) twice.

> 
> Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
> ---
>  .../bindings/watchdog/xlnx,versal-wwdt.yaml   | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml b/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
> new file mode 100644
> index 000000000000..986455efa6f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/xlnx,versal-wwdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Versal window watchdog timer controller
> +
> +maintainers:
> +  - Neeli Srinivas <srinivas.neeli@amd.com>
> +
> +description:
> +  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.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - xlnx,versal-wwdt-1.0

1.0 versions feel made up. Is this a soft IP, because that is really the 
only place we use version numbers. More generally, version numbers need 
to correspond to something.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  timeout-sec: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        watchdog@fd4d0000 {
> +           compatible = "xlnx,versal-wwdt-1.0";
> +           reg = <0xfd4d0000 0x10000>;
> +           clocks = <&clock25>;
> +           timeout-sec = <30>;
> +        };
> +...
> -- 
> 2.17.1
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device
  2022-09-27 11:02 ` [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device Srinivas Neeli
  2022-09-29 22:20   ` Rob Herring
@ 2022-09-30 10:31   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-09-30 10:31 UTC (permalink / raw)
  To: Srinivas Neeli, wim, linux, shubhrajyoti.datta, michal.simek,
	robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

On 27/09/2022 13:02, Srinivas Neeli wrote:
> Add documentation for the binding of window watchdog device.
> 
> Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
> ---
>  .../bindings/watchdog/xlnx,versal-wwdt.yaml   | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml b/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
> new file mode 100644
> index 000000000000..986455efa6f4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/xlnx,versal-wwdt.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/xlnx,versal-wwdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Versal window watchdog timer controller
> +
> +maintainers:
> +  - Neeli Srinivas <srinivas.neeli@amd.com>
> +
> +description:
> +  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.
> +

Missing ref to watchdog.

> +properties:
> +  compatible:
> +    enum:
> +      - xlnx,versal-wwdt-1.0
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  timeout-sec: true

This can be dropped.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +additionalProperties: false

and this then can be unevaluatedProperties:false

> +
> +examples:
> +  - |
> +        watchdog@fd4d0000 {

Use 4 spaces for example indentation.

> +           compatible = "xlnx,versal-wwdt-1.0";
> +           reg = <0xfd4d0000 0x10000>;
> +           clocks = <&clock25>;
> +           timeout-sec = <30>;
> +        };
> +...

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-09-27 11:02 ` [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support Srinivas Neeli
@ 2022-09-30 10:35   ` Krzysztof Kozlowski
  2022-10-02 16:06     ` Guenter Roeck
  2022-10-02 16:29     ` Guenter Roeck
  2022-10-02 16:25   ` Guenter Roeck
  1 sibling, 2 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-09-30 10:35 UTC (permalink / raw)
  To: Srinivas Neeli, wim, linux, shubhrajyoti.datta, michal.simek,
	robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

On 27/09/2022 13:02, Srinivas Neeli wrote:
> +
> +static void xwwdt_clk_disable_unprepare(void *data)
> +{
> +	clk_disable_unprepare(data);

If watchdog is stopped and then device unbound, don't you have double
disable? IOW, where is matching clk_enable?

> +}
> +
> +static const struct watchdog_info xilinx_wwdt_ident = {
> +	.options = WDIOF_KEEPALIVEPING |
> +		WDIOF_SETTIMEOUT,
> +	.firmware_version = 1,
> +	.identity = "xlnx_window watchdog",
> +};
> +
> +static const struct watchdog_ops xilinx_wwdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = xilinx_wwdt_start,
> +	.stop = xilinx_wwdt_stop,
> +	.set_timeout = xilinx_wwdt_set_timeout,
> +	.ping = xilinx_wwdt_keepalive,
> +};
> +
> +static int xwwdt_probe(struct platform_device *pdev)
> +{
> +	struct watchdog_device *xilinx_wwdt_wdd;
> +	struct device *dev = &pdev->dev;
> +	struct xwwdt_device *xdev;
> +	int ret;
> +
> +	xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL);
> +	if (!xdev)
> +		return -ENOMEM;
> +
> +	xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
> +	xilinx_wwdt_wdd->info = &xilinx_wwdt_ident;
> +	xilinx_wwdt_wdd->ops = &xilinx_wwdt_ops;
> +	xilinx_wwdt_wdd->parent = dev;
> +
> +	xdev->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(xdev->base))
> +		return PTR_ERR(xdev->base);
> +
> +	xdev->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(xdev->clk))
> +		return PTR_ERR(xdev->clk);
> +
> +	xdev->freq = clk_get_rate(xdev->clk);
> +	if (!xdev->freq)
> +		return -EINVAL;
> +
> +	ret = clk_prepare_enable(xdev->clk);
> +	if (ret) {
> +		dev_err(dev, "unable to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, xwwdt_clk_disable_unprepare,
> +				       xdev->clk);
> +	if (ret)
> +		return ret;
> +
> +	xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
> +	xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
> +	xilinx_wwdt_wdd->max_timeout = XWWDT_MAX_TIMEOUT;
> +
> +	ret = watchdog_init_timeout(xilinx_wwdt_wdd,
> +				    xwwdt_timeout, &pdev->dev);
> +	if (ret)
> +		dev_info(&pdev->dev, "Configured default timeout value\n");
> +
> +	spin_lock_init(&xdev->spinlock);
> +	watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
> +
> +	ret = devm_watchdog_register_device(dev, xilinx_wwdt_wdd);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "Xilinx window watchdog Timer with timeout %ds\n",
> +		 xilinx_wwdt_wdd->timeout);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xwwdt_of_match[] = {
> +	{ .compatible = "xlnx,versal-wwdt-1.0", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xwwdt_of_match);
> +
> +static struct platform_driver xwwdt_driver = {
> +	.probe = xwwdt_probe,
> +	.driver = {
> +		.name = "Xilinx window watchdog",

Do you see spaces in other names of drivers?

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-09-30 10:35   ` Krzysztof Kozlowski
@ 2022-10-02 16:06     ` Guenter Roeck
  2022-10-02 16:29     ` Guenter Roeck
  1 sibling, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2022-10-02 16:06 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Srinivas Neeli, wim, shubhrajyoti.datta,
	michal.simek, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

On 9/30/22 03:35, Krzysztof Kozlowski wrote:
> On 27/09/2022 13:02, Srinivas Neeli wrote:
>> +
>> +static void xwwdt_clk_disable_unprepare(void *data)
>> +{
>> +	clk_disable_unprepare(data);
> 
> If watchdog is stopped and then device unbound, don't you have double
> disable? IOW, where is matching clk_enable?
> 

See clok_prepare_enable() in the probe function. This kind of code
is quite common in watchdog drivers. Alternative is to have a remove
function and call clk_disable_unprepare() from there. The result is
the same, and the code here is preferred.

Can you be more specific with your concerns ? This is quite common
code for watchdog drivers, so any concern would be important to
understand.

>> +}
>> +
>> +static const struct watchdog_info xilinx_wwdt_ident = {
>> +	.options = WDIOF_KEEPALIVEPING |
>> +		WDIOF_SETTIMEOUT,
>> +	.firmware_version = 1,
>> +	.identity = "xlnx_window watchdog",
>> +};
>> +
>> +static const struct watchdog_ops xilinx_wwdt_ops = {
>> +	.owner = THIS_MODULE,
>> +	.start = xilinx_wwdt_start,
>> +	.stop = xilinx_wwdt_stop,
>> +	.set_timeout = xilinx_wwdt_set_timeout,
>> +	.ping = xilinx_wwdt_keepalive,
>> +};
>> +
>> +static int xwwdt_probe(struct platform_device *pdev)
>> +{
>> +	struct watchdog_device *xilinx_wwdt_wdd;
>> +	struct device *dev = &pdev->dev;
>> +	struct xwwdt_device *xdev;
>> +	int ret;
>> +
>> +	xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL);
>> +	if (!xdev)
>> +		return -ENOMEM;
>> +
>> +	xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
>> +	xilinx_wwdt_wdd->info = &xilinx_wwdt_ident;
>> +	xilinx_wwdt_wdd->ops = &xilinx_wwdt_ops;
>> +	xilinx_wwdt_wdd->parent = dev;
>> +
>> +	xdev->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(xdev->base))
>> +		return PTR_ERR(xdev->base);
>> +
>> +	xdev->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(xdev->clk))
>> +		return PTR_ERR(xdev->clk);
>> +
>> +	xdev->freq = clk_get_rate(xdev->clk);
>> +	if (!xdev->freq)
>> +		return -EINVAL;
>> +
>> +	ret = clk_prepare_enable(xdev->clk);
>> +	if (ret) {
>> +		dev_err(dev, "unable to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_add_action_or_reset(dev, xwwdt_clk_disable_unprepare,
>> +				       xdev->clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
>> +	xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
>> +	xilinx_wwdt_wdd->max_timeout = XWWDT_MAX_TIMEOUT;
>> +
>> +	ret = watchdog_init_timeout(xilinx_wwdt_wdd,
>> +				    xwwdt_timeout, &pdev->dev);
>> +	if (ret)
>> +		dev_info(&pdev->dev, "Configured default timeout value\n");
>> +
>> +	spin_lock_init(&xdev->spinlock);
>> +	watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
>> +
>> +	ret = devm_watchdog_register_device(dev, xilinx_wwdt_wdd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_info(dev, "Xilinx window watchdog Timer with timeout %ds\n",
>> +		 xilinx_wwdt_wdd->timeout);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id xwwdt_of_match[] = {
>> +	{ .compatible = "xlnx,versal-wwdt-1.0", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, xwwdt_of_match);
>> +
>> +static struct platform_driver xwwdt_driver = {
>> +	.probe = xwwdt_probe,
>> +	.driver = {
>> +		.name = "Xilinx window watchdog",
> 
> Do you see spaces in other names of drivers?
> 

Easier to say that platform driver names must not include spaces (or dashes,
for that matter).

Thanks,
Guenter

> Best regards,
> Krzysztof
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-09-27 11:02 ` [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support Srinivas Neeli
  2022-09-30 10:35   ` Krzysztof Kozlowski
@ 2022-10-02 16:25   ` Guenter Roeck
  2022-10-11  6:27     ` Neeli, Srinivas
  1 sibling, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2022-10-02 16:25 UTC (permalink / raw)
  To: Srinivas Neeli
  Cc: wim, shubhrajyoti.datta, michal.simek, robh+dt,
	krzysztof.kozlowski+dt, linux-kernel, linux-watchdog,
	linux-arm-kernel, devicetree, git

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@amd.com>
> ---
>  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.

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

> +
> +/**
> + * 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).

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

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

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

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

> +
> +	wdd->timeout = new_time;
> +
> +	return 0;
> +}
> +
> +static int xilinx_wwdt_stop(struct watchdog_device *wdd)
> +{
> +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> +	struct watchdog_device *xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
> +
> +	if (watchdog_active(xilinx_wwdt_wdd)) {
> +		if (!is_wwdt_in_closed_window(wdd)) {
> +			dev_warn(xilinx_wwdt_wdd->parent, "timer in closed window");
> +			return -EPERM;
> +		}
> +	}
> +
> +	spin_lock(&xdev->spinlock);
> +
> +	iowrite32(XWWDT_MWR_MASK, xdev->base + XWWDT_MWR_OFFSET);
> +
> +	/* Disable the Window watchdog timer */
> +	iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base + XWWDT_ESR_OFFSET);
> +
> +	spin_unlock(&xdev->spinlock);
> +
> +	clk_disable(xdev->clk);

This doesn't work. The start function doesn't enable the clock; it is
enabled in the probe function. If you want to enable the clock
dynamically, you'll have to enable it in the start function and make sure
that it is stopped when unloading the driver (you can't use the devm
function in this case). You'll also need to make sure that the unprepare
function is called when unloading the driver.

> +
> +	dev_dbg(xilinx_wwdt_wdd->parent, "Watchdog Stopped!\n");
> +
> +	return 0;
> +}
> +
> +static void xwwdt_clk_disable_unprepare(void *data)
> +{
> +	clk_disable_unprepare(data);
> +}
> +
> +static const struct watchdog_info xilinx_wwdt_ident = {
> +	.options = WDIOF_KEEPALIVEPING |
> +		WDIOF_SETTIMEOUT,
> +	.firmware_version = 1,
> +	.identity = "xlnx_window watchdog",
> +};
> +
> +static const struct watchdog_ops xilinx_wwdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = xilinx_wwdt_start,
> +	.stop = xilinx_wwdt_stop,
> +	.set_timeout = xilinx_wwdt_set_timeout,
> +	.ping = xilinx_wwdt_keepalive,
> +};
> +
> +static int xwwdt_probe(struct platform_device *pdev)
> +{
> +	struct watchdog_device *xilinx_wwdt_wdd;
> +	struct device *dev = &pdev->dev;
> +	struct xwwdt_device *xdev;
> +	int ret;
> +
> +	xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL);
> +	if (!xdev)
> +		return -ENOMEM;
> +
> +	xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
> +	xilinx_wwdt_wdd->info = &xilinx_wwdt_ident;
> +	xilinx_wwdt_wdd->ops = &xilinx_wwdt_ops;
> +	xilinx_wwdt_wdd->parent = dev;
> +
> +	xdev->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(xdev->base))
> +		return PTR_ERR(xdev->base);
> +
> +	xdev->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(xdev->clk))
> +		return PTR_ERR(xdev->clk);
> +
> +	xdev->freq = clk_get_rate(xdev->clk);
> +	if (!xdev->freq)
> +		return -EINVAL;
> +
> +	ret = clk_prepare_enable(xdev->clk);
> +	if (ret) {
> +		dev_err(dev, "unable to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, xwwdt_clk_disable_unprepare,
> +				       xdev->clk);
> +	if (ret)
> +		return ret;
> +
> +	xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
> +	xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
> +	xilinx_wwdt_wdd->max_timeout = XWWDT_MAX_TIMEOUT;
> +
> +	ret = watchdog_init_timeout(xilinx_wwdt_wdd,
> +				    xwwdt_timeout, &pdev->dev);
> +	if (ret)
> +		dev_info(&pdev->dev, "Configured default timeout value\n");
> +
> +	spin_lock_init(&xdev->spinlock);
> +	watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
> +
> +	ret = devm_watchdog_register_device(dev, xilinx_wwdt_wdd);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "Xilinx window watchdog Timer with timeout %ds\n",
> +		 xilinx_wwdt_wdd->timeout);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id xwwdt_of_match[] = {
> +	{ .compatible = "xlnx,versal-wwdt-1.0", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xwwdt_of_match);
> +
> +static struct platform_driver xwwdt_driver = {
> +	.probe = xwwdt_probe,
> +	.driver = {
> +		.name = "Xilinx window watchdog",
> +		.of_match_table = xwwdt_of_match,
> +	},
> +};
> +
> +module_platform_driver(xwwdt_driver);
> +
> +MODULE_AUTHOR("Neeli Srinivas <srinivas.neeli@amd.com>");
> +MODULE_DESCRIPTION("Xilinx window watchdog driver");
> +MODULE_LICENSE("GPL");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-09-30 10:35   ` Krzysztof Kozlowski
  2022-10-02 16:06     ` Guenter Roeck
@ 2022-10-02 16:29     ` Guenter Roeck
  2022-10-03  7:00       ` Krzysztof Kozlowski
  1 sibling, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2022-10-02 16:29 UTC (permalink / raw)
  To: Krzysztof Kozlowski, Srinivas Neeli, wim, shubhrajyoti.datta,
	michal.simek, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

On 9/30/22 03:35, Krzysztof Kozlowski wrote:
> On 27/09/2022 13:02, Srinivas Neeli wrote:
>> +
>> +static void xwwdt_clk_disable_unprepare(void *data)
>> +{
>> +	clk_disable_unprepare(data);
> 
> If watchdog is stopped and then device unbound, don't you have double
> disable? IOW, where is matching clk_enable?
> 

After looking into the entire driver code, I see the problem:
clk_disable() is called in the stop function. That is wrong;
if the clock is disabled in the stop function, it needs to be
enabled in the start function (which is not currently the case).
In that situation, using the devm_ callback when unloading the
driver isn't possible (or, rather, would require checking if the
clock is enabled before disabling it).

Thanks,
Guenter

>> +}
>> +
>> +static const struct watchdog_info xilinx_wwdt_ident = {
>> +	.options = WDIOF_KEEPALIVEPING |
>> +		WDIOF_SETTIMEOUT,
>> +	.firmware_version = 1,
>> +	.identity = "xlnx_window watchdog",
>> +};
>> +
>> +static const struct watchdog_ops xilinx_wwdt_ops = {
>> +	.owner = THIS_MODULE,
>> +	.start = xilinx_wwdt_start,
>> +	.stop = xilinx_wwdt_stop,
>> +	.set_timeout = xilinx_wwdt_set_timeout,
>> +	.ping = xilinx_wwdt_keepalive,
>> +};
>> +
>> +static int xwwdt_probe(struct platform_device *pdev)
>> +{
>> +	struct watchdog_device *xilinx_wwdt_wdd;
>> +	struct device *dev = &pdev->dev;
>> +	struct xwwdt_device *xdev;
>> +	int ret;
>> +
>> +	xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL);
>> +	if (!xdev)
>> +		return -ENOMEM;
>> +
>> +	xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
>> +	xilinx_wwdt_wdd->info = &xilinx_wwdt_ident;
>> +	xilinx_wwdt_wdd->ops = &xilinx_wwdt_ops;
>> +	xilinx_wwdt_wdd->parent = dev;
>> +
>> +	xdev->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(xdev->base))
>> +		return PTR_ERR(xdev->base);
>> +
>> +	xdev->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(xdev->clk))
>> +		return PTR_ERR(xdev->clk);
>> +
>> +	xdev->freq = clk_get_rate(xdev->clk);
>> +	if (!xdev->freq)
>> +		return -EINVAL;
>> +
>> +	ret = clk_prepare_enable(xdev->clk);
>> +	if (ret) {
>> +		dev_err(dev, "unable to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_add_action_or_reset(dev, xwwdt_clk_disable_unprepare,
>> +				       xdev->clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
>> +	xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
>> +	xilinx_wwdt_wdd->max_timeout = XWWDT_MAX_TIMEOUT;
>> +
>> +	ret = watchdog_init_timeout(xilinx_wwdt_wdd,
>> +				    xwwdt_timeout, &pdev->dev);
>> +	if (ret)
>> +		dev_info(&pdev->dev, "Configured default timeout value\n");
>> +
>> +	spin_lock_init(&xdev->spinlock);
>> +	watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
>> +
>> +	ret = devm_watchdog_register_device(dev, xilinx_wwdt_wdd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev_info(dev, "Xilinx window watchdog Timer with timeout %ds\n",
>> +		 xilinx_wwdt_wdd->timeout);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id xwwdt_of_match[] = {
>> +	{ .compatible = "xlnx,versal-wwdt-1.0", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, xwwdt_of_match);
>> +
>> +static struct platform_driver xwwdt_driver = {
>> +	.probe = xwwdt_probe,
>> +	.driver = {
>> +		.name = "Xilinx window watchdog",
> 
> Do you see spaces in other names of drivers?
> 
> Best regards,
> Krzysztof
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-10-02 16:29     ` Guenter Roeck
@ 2022-10-03  7:00       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2022-10-03  7:00 UTC (permalink / raw)
  To: Guenter Roeck, Srinivas Neeli, wim, shubhrajyoti.datta,
	michal.simek, robh+dt, krzysztof.kozlowski+dt
  Cc: linux-kernel, linux-watchdog, linux-arm-kernel, devicetree, git

On 02/10/2022 18:29, Guenter Roeck wrote:
> On 9/30/22 03:35, Krzysztof Kozlowski wrote:
>> On 27/09/2022 13:02, Srinivas Neeli wrote:
>>> +
>>> +static void xwwdt_clk_disable_unprepare(void *data)
>>> +{
>>> +	clk_disable_unprepare(data);
>>
>> If watchdog is stopped and then device unbound, don't you have double
>> disable? IOW, where is matching clk_enable?
>>
> 
> After looking into the entire driver code, I see the problem:
> clk_disable() is called in the stop function. That is wrong;
> if the clock is disabled in the stop function, it needs to be

Yep. clk_disable and clk_enable are always paired. In probe they are
pared with remove() or reset-action. This one is in stop(), so it does
not have its pair.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-10-02 16:25   ` Guenter Roeck
@ 2022-10-11  6:27     ` Neeli, Srinivas
  2022-11-03 16:51       ` Neeli, Srinivas
  0 siblings, 1 reply; 17+ messages in thread
From: Neeli, Srinivas @ 2022-10-11  6:27 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: wim, Datta, Shubhrajyoti, Simek, Michal, robh+dt,
	krzysztof.kozlowski+dt, linux-kernel, linux-watchdog,
	linux-arm-kernel, devicetree, git (AMD-Xilinx)

Hi,

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Sunday, October 2, 2022 9:55 PM
> To: Neeli, Srinivas <srinivas.neeli@amd.com>
> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; Simek, Michal <michal.simek@amd.com>;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>
> 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@amd.com>
> > ---
> >  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.

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

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

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

> 
> > +
> > +	wdd->timeout = new_time;
> > +
> > +	return 0;
> > +}
> > +
> > +static int xilinx_wwdt_stop(struct watchdog_device *wdd) {
> > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > +	struct watchdog_device *xilinx_wwdt_wdd = &xdev-
> >xilinx_wwdt_wdd;
> > +
> > +	if (watchdog_active(xilinx_wwdt_wdd)) {
> > +		if (!is_wwdt_in_closed_window(wdd)) {
> > +			dev_warn(xilinx_wwdt_wdd->parent, "timer in
> closed window");
> > +			return -EPERM;
> > +		}
> > +	}
> > +
> > +	spin_lock(&xdev->spinlock);
> > +
> > +	iowrite32(XWWDT_MWR_MASK, xdev->base +
> XWWDT_MWR_OFFSET);
> > +
> > +	/* Disable the Window watchdog timer */
> > +	iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base +
> XWWDT_ESR_OFFSET);
> > +
> > +	spin_unlock(&xdev->spinlock);
> > +
> > +	clk_disable(xdev->clk);
> 
> This doesn't work. The start function doesn't enable the clock; it is enabled in
> the probe function. If you want to enable the clock dynamically, you'll have
> to enable it in the start function and make sure that it is stopped when
> unloading the driver (you can't use the devm function in this case). You'll also
> need to make sure that the unprepare function is called when unloading the
> driver.
> 

Accepted and will update in V2.

Thanks
Neeli Srinivas
> > +
> > +	dev_dbg(xilinx_wwdt_wdd->parent, "Watchdog Stopped!\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static void xwwdt_clk_disable_unprepare(void *data) {
> > +	clk_disable_unprepare(data);
> > +}
> > +
> > +static const struct watchdog_info xilinx_wwdt_ident = {
> > +	.options = WDIOF_KEEPALIVEPING |
> > +		WDIOF_SETTIMEOUT,
> > +	.firmware_version = 1,
> > +	.identity = "xlnx_window watchdog",
> > +};
> > +
> > +static const struct watchdog_ops xilinx_wwdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = xilinx_wwdt_start,
> > +	.stop = xilinx_wwdt_stop,
> > +	.set_timeout = xilinx_wwdt_set_timeout,
> > +	.ping = xilinx_wwdt_keepalive,
> > +};
> > +
> > +static int xwwdt_probe(struct platform_device *pdev) {
> > +	struct watchdog_device *xilinx_wwdt_wdd;
> > +	struct device *dev = &pdev->dev;
> > +	struct xwwdt_device *xdev;
> > +	int ret;
> > +
> > +	xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL);
> > +	if (!xdev)
> > +		return -ENOMEM;
> > +
> > +	xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
> > +	xilinx_wwdt_wdd->info = &xilinx_wwdt_ident;
> > +	xilinx_wwdt_wdd->ops = &xilinx_wwdt_ops;
> > +	xilinx_wwdt_wdd->parent = dev;
> > +
> > +	xdev->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(xdev->base))
> > +		return PTR_ERR(xdev->base);
> > +
> > +	xdev->clk = devm_clk_get(dev, NULL);
> > +	if (IS_ERR(xdev->clk))
> > +		return PTR_ERR(xdev->clk);
> > +
> > +	xdev->freq = clk_get_rate(xdev->clk);
> > +	if (!xdev->freq)
> > +		return -EINVAL;
> > +
> > +	ret = clk_prepare_enable(xdev->clk);
> > +	if (ret) {
> > +		dev_err(dev, "unable to enable clock\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_add_action_or_reset(dev,
> xwwdt_clk_disable_unprepare,
> > +				       xdev->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
> > +	xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
> > +	xilinx_wwdt_wdd->max_timeout = XWWDT_MAX_TIMEOUT;
> > +
> > +	ret = watchdog_init_timeout(xilinx_wwdt_wdd,
> > +				    xwwdt_timeout, &pdev->dev);
> > +	if (ret)
> > +		dev_info(&pdev->dev, "Configured default timeout
> value\n");
> > +
> > +	spin_lock_init(&xdev->spinlock);
> > +	watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
> > +
> > +	ret = devm_watchdog_register_device(dev, xilinx_wwdt_wdd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_info(dev, "Xilinx window watchdog Timer with timeout %ds\n",
> > +		 xilinx_wwdt_wdd->timeout);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id xwwdt_of_match[] = {
> > +	{ .compatible = "xlnx,versal-wwdt-1.0", },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, xwwdt_of_match);
> > +
> > +static struct platform_driver xwwdt_driver = {
> > +	.probe = xwwdt_probe,
> > +	.driver = {
> > +		.name = "Xilinx window watchdog",
> > +		.of_match_table = xwwdt_of_match,
> > +	},
> > +};
> > +
> > +module_platform_driver(xwwdt_driver);
> > +
> > +MODULE_AUTHOR("Neeli Srinivas <srinivas.neeli@amd.com>");
> > +MODULE_DESCRIPTION("Xilinx window watchdog driver");
> > +MODULE_LICENSE("GPL");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-10-11  6:27     ` Neeli, Srinivas
@ 2022-11-03 16:51       ` Neeli, Srinivas
  2022-11-03 17:24         ` Guenter Roeck
  0 siblings, 1 reply; 17+ messages in thread
From: Neeli, Srinivas @ 2022-11-03 16:51 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: wim, Datta, Shubhrajyoti, Simek, Michal, robh+dt,
	krzysztof.kozlowski+dt, linux-kernel, linux-watchdog,
	linux-arm-kernel, devicetree, git (AMD-Xilinx)

HI Guenter,

> -----Original Message-----
> From: Neeli, Srinivas <srinivas.neeli@amd.com>
> Sent: Tuesday, October 11, 2022 11:57 AM
> To: Guenter Roeck <linux@roeck-us.net>
> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; Simek, Michal <michal.simek@amd.com>;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>
> Subject: RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
> watchdog support
> 
> Hi,
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > Sent: Sunday, October 2, 2022 9:55 PM
> > To: Neeli, Srinivas <srinivas.neeli@amd.com>
> > Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> > <shubhrajyoti.datta@amd.com>; Simek, Michal
> <michal.simek@amd.com>;
> > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> > kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; devicetree@vger.kernel.org; git
> > (AMD-Xilinx) <git@amd.com>
> > 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@amd.com>
> > > ---
> > >  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.
> 
> >
> > > +		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.
> 
> >
> > > +	}
> > > +
> > > +	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.
> 
> >
> > > +
> > > +	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.
> 
> >
> > > +
> > > +	wdd->timeout = new_time;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int xilinx_wwdt_stop(struct watchdog_device *wdd) {
> > > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > > +	struct watchdog_device *xilinx_wwdt_wdd = &xdev-
> > >xilinx_wwdt_wdd;
> > > +
> > > +	if (watchdog_active(xilinx_wwdt_wdd)) {
> > > +		if (!is_wwdt_in_closed_window(wdd)) {
> > > +			dev_warn(xilinx_wwdt_wdd->parent, "timer in
> > closed window");
> > > +			return -EPERM;
> > > +		}
> > > +	}
> > > +
> > > +	spin_lock(&xdev->spinlock);
> > > +
> > > +	iowrite32(XWWDT_MWR_MASK, xdev->base +
> > XWWDT_MWR_OFFSET);
> > > +
> > > +	/* Disable the Window watchdog timer */
> > > +	iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base +
> > XWWDT_ESR_OFFSET);
> > > +
> > > +	spin_unlock(&xdev->spinlock);
> > > +
> > > +	clk_disable(xdev->clk);
> >
> > This doesn't work. The start function doesn't enable the clock; it is
> > enabled in the probe function. If you want to enable the clock
> > dynamically, you'll have to enable it in the start function and make
> > sure that it is stopped when unloading the driver (you can't use the
> > devm function in this case). You'll also need to make sure that the
> > unprepare function is called when unloading the driver.
> >
> 
> Accepted and will update in V2.
> 
> Thanks
> Neeli Srinivas

Could you please let me know your thoughts on "one line comment summary".

Thanks
Neeli Srinivas
> > > +
> > > +	dev_dbg(xilinx_wwdt_wdd->parent, "Watchdog Stopped!\n");
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void xwwdt_clk_disable_unprepare(void *data) {
> > > +	clk_disable_unprepare(data);
> > > +}
> > > +
> > > +static const struct watchdog_info xilinx_wwdt_ident = {
> > > +	.options = WDIOF_KEEPALIVEPING |
> > > +		WDIOF_SETTIMEOUT,
> > > +	.firmware_version = 1,
> > > +	.identity = "xlnx_window watchdog", };
> > > +
> > > +static const struct watchdog_ops xilinx_wwdt_ops = {
> > > +	.owner = THIS_MODULE,
> > > +	.start = xilinx_wwdt_start,
> > > +	.stop = xilinx_wwdt_stop,
> > > +	.set_timeout = xilinx_wwdt_set_timeout,
> > > +	.ping = xilinx_wwdt_keepalive,
> > > +};
> > > +
> > > +static int xwwdt_probe(struct platform_device *pdev) {
> > > +	struct watchdog_device *xilinx_wwdt_wdd;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct xwwdt_device *xdev;
> > > +	int ret;
> > > +
> > > +	xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL);
> > > +	if (!xdev)
> > > +		return -ENOMEM;
> > > +
> > > +	xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
> > > +	xilinx_wwdt_wdd->info = &xilinx_wwdt_ident;
> > > +	xilinx_wwdt_wdd->ops = &xilinx_wwdt_ops;
> > > +	xilinx_wwdt_wdd->parent = dev;
> > > +
> > > +	xdev->base = devm_platform_ioremap_resource(pdev, 0);
> > > +	if (IS_ERR(xdev->base))
> > > +		return PTR_ERR(xdev->base);
> > > +
> > > +	xdev->clk = devm_clk_get(dev, NULL);
> > > +	if (IS_ERR(xdev->clk))
> > > +		return PTR_ERR(xdev->clk);
> > > +
> > > +	xdev->freq = clk_get_rate(xdev->clk);
> > > +	if (!xdev->freq)
> > > +		return -EINVAL;
> > > +
> > > +	ret = clk_prepare_enable(xdev->clk);
> > > +	if (ret) {
> > > +		dev_err(dev, "unable to enable clock\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = devm_add_action_or_reset(dev,
> > xwwdt_clk_disable_unprepare,
> > > +				       xdev->clk);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
> > > +	xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
> > > +	xilinx_wwdt_wdd->max_timeout = XWWDT_MAX_TIMEOUT;
> > > +
> > > +	ret = watchdog_init_timeout(xilinx_wwdt_wdd,
> > > +				    xwwdt_timeout, &pdev->dev);
> > > +	if (ret)
> > > +		dev_info(&pdev->dev, "Configured default timeout
> > value\n");
> > > +
> > > +	spin_lock_init(&xdev->spinlock);
> > > +	watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
> > > +
> > > +	ret = devm_watchdog_register_device(dev, xilinx_wwdt_wdd);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	dev_info(dev, "Xilinx window watchdog Timer with timeout %ds\n",
> > > +		 xilinx_wwdt_wdd->timeout);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id xwwdt_of_match[] = {
> > > +	{ .compatible = "xlnx,versal-wwdt-1.0", },
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, xwwdt_of_match);
> > > +
> > > +static struct platform_driver xwwdt_driver = {
> > > +	.probe = xwwdt_probe,
> > > +	.driver = {
> > > +		.name = "Xilinx window watchdog",
> > > +		.of_match_table = xwwdt_of_match,
> > > +	},
> > > +};
> > > +
> > > +module_platform_driver(xwwdt_driver);
> > > +
> > > +MODULE_AUTHOR("Neeli Srinivas <srinivas.neeli@amd.com>");
> > > +MODULE_DESCRIPTION("Xilinx window watchdog driver");
> > > +MODULE_LICENSE("GPL");

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-11-03 16:51       ` Neeli, Srinivas
@ 2022-11-03 17:24         ` Guenter Roeck
  2022-11-06 15:16           ` Neeli, Srinivas
  0 siblings, 1 reply; 17+ messages in thread
From: Guenter Roeck @ 2022-11-03 17:24 UTC (permalink / raw)
  To: Neeli, Srinivas
  Cc: wim, Datta, Shubhrajyoti, Simek, Michal, robh+dt,
	krzysztof.kozlowski+dt, linux-kernel, linux-watchdog,
	linux-arm-kernel, devicetree, git (AMD-Xilinx)

On Thu, Nov 03, 2022 at 04:51:14PM +0000, Neeli, Srinivas wrote:
> HI Guenter,
> 
> > -----Original Message-----
> > From: Neeli, Srinivas <srinivas.neeli@amd.com>
> > Sent: Tuesday, October 11, 2022 11:57 AM
> > To: Guenter Roeck <linux@roeck-us.net>
> > Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> > <shubhrajyoti.datta@amd.com>; Simek, Michal <michal.simek@amd.com>;
> > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> > kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> > <git@amd.com>
> > Subject: RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
> > watchdog support
> > 
> > Hi,
> > 
> > > -----Original Message-----
> > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> > > Sent: Sunday, October 2, 2022 9:55 PM
> > > To: Neeli, Srinivas <srinivas.neeli@amd.com>
> > > Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> > > <shubhrajyoti.datta@amd.com>; Simek, Michal
> > <michal.simek@amd.com>;
> > > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> > > kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; devicetree@vger.kernel.org; git
> > > (AMD-Xilinx) <git@amd.com>
> > > 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@amd.com>
> > > > ---
> > > >  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.
> > 
> > >
> > > > +		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.
> > 
> > >
> > > > +	}
> > > > +
> > > > +	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.
> > 
> > >
> > > > +
> > > > +	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.
> > 
> > >
> > > > +
> > > > +	wdd->timeout = new_time;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int xilinx_wwdt_stop(struct watchdog_device *wdd) {
> > > > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > > > +	struct watchdog_device *xilinx_wwdt_wdd = &xdev-
> > > >xilinx_wwdt_wdd;
> > > > +
> > > > +	if (watchdog_active(xilinx_wwdt_wdd)) {
> > > > +		if (!is_wwdt_in_closed_window(wdd)) {
> > > > +			dev_warn(xilinx_wwdt_wdd->parent, "timer in
> > > closed window");
> > > > +			return -EPERM;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	spin_lock(&xdev->spinlock);
> > > > +
> > > > +	iowrite32(XWWDT_MWR_MASK, xdev->base +
> > > XWWDT_MWR_OFFSET);
> > > > +
> > > > +	/* Disable the Window watchdog timer */
> > > > +	iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base +
> > > XWWDT_ESR_OFFSET);
> > > > +
> > > > +	spin_unlock(&xdev->spinlock);
> > > > +
> > > > +	clk_disable(xdev->clk);
> > >
> > > This doesn't work. The start function doesn't enable the clock; it is
> > > enabled in the probe function. If you want to enable the clock
> > > dynamically, you'll have to enable it in the start function and make
> > > sure that it is stopped when unloading the driver (you can't use the
> > > devm function in this case). You'll also need to make sure that the
> > > unprepare function is called when unloading the driver.
> > >
> > 
> > Accepted and will update in V2.
> > 
> > Thanks
> > Neeli Srinivas
> 
> Could you please let me know your thoughts on "one line comment summary".
> 

Sorry, I have no idea what you refer to. Searching for any of the words in
"one line comment summary" in this patch doesn't give me a hint either.

Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-11-03 17:24         ` Guenter Roeck
@ 2022-11-06 15:16           ` Neeli, Srinivas
  2022-12-01 11:08             ` Michal Simek
  0 siblings, 1 reply; 17+ messages in thread
From: Neeli, Srinivas @ 2022-11-06 15:16 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: wim, Datta, Shubhrajyoti, Simek, Michal, robh+dt,
	krzysztof.kozlowski+dt, linux-kernel, linux-watchdog,
	linux-arm-kernel, devicetree, git (AMD-Xilinx)

Hi Guenter,

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Thursday, November 3, 2022 10:55 PM
> To: Neeli, Srinivas <srinivas.neeli@amd.com>
> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; Simek, Michal <michal.simek@amd.com>;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>
> 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@amd.com>
> > > Sent: Tuesday, October 11, 2022 11:57 AM
> > > To: Guenter Roeck <linux@roeck-us.net>
> > > Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> > > <shubhrajyoti.datta@amd.com>; Simek, Michal
> <michal.simek@amd.com>;
> > > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> > > kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; devicetree@vger.kernel.org; git
> > > (AMD-Xilinx) <git@amd.com>
> > > Subject: RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
> > > watchdog support
> > >
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> > > > Sent: Sunday, October 2, 2022 9:55 PM
> > > > To: Neeli, Srinivas <srinivas.neeli@amd.com>
> > > > Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> > > > <shubhrajyoti.datta@amd.com>; Simek, Michal
> > > <michal.simek@amd.com>;
> > > > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> > > > kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> > > > kernel@lists.infradead.org; devicetree@vger.kernel.org; git
> > > > (AMD-Xilinx) <git@amd.com>
> > > > 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@amd.com>
> > > > > ---
> > > > >  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.

> > > >
> > > > > +
> > > > > +	wdd->timeout = new_time;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int xilinx_wwdt_stop(struct watchdog_device *wdd) {
> > > > > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > > > > +	struct watchdog_device *xilinx_wwdt_wdd = &xdev-
> > > > >xilinx_wwdt_wdd;
> > > > > +
> > > > > +	if (watchdog_active(xilinx_wwdt_wdd)) {
> > > > > +		if (!is_wwdt_in_closed_window(wdd)) {
> > > > > +			dev_warn(xilinx_wwdt_wdd->parent, "timer
> in
> > > > closed window");
> > > > > +			return -EPERM;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	spin_lock(&xdev->spinlock);
> > > > > +
> > > > > +	iowrite32(XWWDT_MWR_MASK, xdev->base +
> > > > XWWDT_MWR_OFFSET);
> > > > > +
> > > > > +	/* Disable the Window watchdog timer */
> > > > > +	iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base +
> > > > XWWDT_ESR_OFFSET);
> > > > > +
> > > > > +	spin_unlock(&xdev->spinlock);
> > > > > +
> > > > > +	clk_disable(xdev->clk);
> > > >
> > > > This doesn't work. The start function doesn't enable the clock; it
> > > > is enabled in the probe function. If you want to enable the clock
> > > > dynamically, you'll have to enable it in the start function and
> > > > make sure that it is stopped when unloading the driver (you can't
> > > > use the devm function in this case). You'll also need to make sure
> > > > that the unprepare function is called when unloading the driver.
> > > >
> > >
> > > Accepted and will update in V2.
> > >
> > > Thanks
> > > Neeli Srinivas
> >
> > Could you please let me know your thoughts on "one line comment
> summary".
> >
> 
> Sorry, I have no idea what you refer to. Searching for any of the words in
> "one line comment summary" in this patch doesn't give me a hint either.
> 
> Guenter
Sorry, it was mistake from my side. Please ignore it.
I responded to all open questions, please suggest your comments.

Thanks
Neeli Srinivas


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-11-06 15:16           ` Neeli, Srinivas
@ 2022-12-01 11:08             ` Michal Simek
  2022-12-01 15:56               ` Guenter Roeck
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Simek @ 2022-12-01 11:08 UTC (permalink / raw)
  To: Neeli, Srinivas, Guenter Roeck
  Cc: wim, Datta, Shubhrajyoti, robh+dt, krzysztof.kozlowski+dt,
	linux-kernel, linux-watchdog, linux-arm-kernel, devicetree,
	git (AMD-Xilinx)

Hi Guenter,

On 11/6/22 16:16, Neeli, Srinivas wrote:
> Hi Guenter,
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Thursday, November 3, 2022 10:55 PM
>> To: Neeli, Srinivas <srinivas.neeli@amd.com>
>> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
>> <shubhrajyoti.datta@amd.com>; Simek, Michal <michal.simek@amd.com>;
>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
>> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
>> kernel@lists.infradead.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
>> <git@amd.com>
>> 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@amd.com>
>>>> Sent: Tuesday, October 11, 2022 11:57 AM
>>>> To: Guenter Roeck <linux@roeck-us.net>
>>>> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
>>>> <shubhrajyoti.datta@amd.com>; Simek, Michal
>> <michal.simek@amd.com>;
>>>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
>>>> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
>>>> kernel@lists.infradead.org; devicetree@vger.kernel.org; git
>>>> (AMD-Xilinx) <git@amd.com>
>>>> Subject: RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
>>>> watchdog support
>>>>
>>>> Hi,
>>>>
>>>>> -----Original Message-----
>>>>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
>> Roeck
>>>>> Sent: Sunday, October 2, 2022 9:55 PM
>>>>> To: Neeli, Srinivas <srinivas.neeli@amd.com>
>>>>> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
>>>>> <shubhrajyoti.datta@amd.com>; Simek, Michal
>>>> <michal.simek@amd.com>;
>>>>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
>>>>> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
>>>>> kernel@lists.infradead.org; devicetree@vger.kernel.org; git
>>>>> (AMD-Xilinx) <git@amd.com>
>>>>> 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@amd.com>
>>>>>> ---
>>>>>>   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.

Thanks,
Michal

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support
  2022-12-01 11:08             ` Michal Simek
@ 2022-12-01 15:56               ` Guenter Roeck
  0 siblings, 0 replies; 17+ messages in thread
From: Guenter Roeck @ 2022-12-01 15:56 UTC (permalink / raw)
  To: Michal Simek, Neeli, Srinivas
  Cc: wim, Datta, Shubhrajyoti, robh+dt, krzysztof.kozlowski+dt,
	linux-kernel, linux-watchdog, linux-arm-kernel, devicetree,
	git (AMD-Xilinx)

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@gmail.com> On Behalf Of Guenter Roeck
>>> Sent: Thursday, November 3, 2022 10:55 PM
>>> To: Neeli, Srinivas <srinivas.neeli@amd.com>
>>> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
>>> <shubhrajyoti.datta@amd.com>; Simek, Michal <michal.simek@amd.com>;
>>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
>>> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
>>> kernel@lists.infradead.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
>>> <git@amd.com>
>>> 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@amd.com>
>>>>> Sent: Tuesday, October 11, 2022 11:57 AM
>>>>> To: Guenter Roeck <linux@roeck-us.net>
>>>>> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
>>>>> <shubhrajyoti.datta@amd.com>; Simek, Michal
>>> <michal.simek@amd.com>;
>>>>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
>>>>> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
>>>>> kernel@lists.infradead.org; devicetree@vger.kernel.org; git
>>>>> (AMD-Xilinx) <git@amd.com>
>>>>> Subject: RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
>>>>> watchdog support
>>>>>
>>>>> Hi,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
>>> Roeck
>>>>>> Sent: Sunday, October 2, 2022 9:55 PM
>>>>>> To: Neeli, Srinivas <srinivas.neeli@amd.com>
>>>>>> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
>>>>>> <shubhrajyoti.datta@amd.com>; Simek, Michal
>>>>> <michal.simek@amd.com>;
>>>>>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
>>>>>> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
>>>>>> kernel@lists.infradead.org; devicetree@vger.kernel.org; git
>>>>>> (AMD-Xilinx) <git@amd.com>
>>>>>> 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@amd.com>
>>>>>>> ---
>>>>>>>   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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-12-01 15:57 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-27 11:02 [PATCH 0/3] watchdog: xilinx_wwdt: Add Versal watchdog support Srinivas Neeli
2022-09-27 11:02 ` [PATCH 1/3] dt-bindings: watchdog: xlnx,versal-wwdt: Add binding documentation for xilinx window watchdog device Srinivas Neeli
2022-09-29 22:20   ` Rob Herring
2022-09-30 10:31   ` Krzysztof Kozlowski
2022-09-27 11:02 ` [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support Srinivas Neeli
2022-09-30 10:35   ` Krzysztof Kozlowski
2022-10-02 16:06     ` Guenter Roeck
2022-10-02 16:29     ` Guenter Roeck
2022-10-03  7:00       ` Krzysztof Kozlowski
2022-10-02 16:25   ` Guenter Roeck
2022-10-11  6:27     ` Neeli, Srinivas
2022-11-03 16:51       ` Neeli, Srinivas
2022-11-03 17:24         ` Guenter Roeck
2022-11-06 15:16           ` Neeli, Srinivas
2022-12-01 11:08             ` Michal Simek
2022-12-01 15:56               ` Guenter Roeck
2022-09-27 11:02 ` [PATCH 3/3] MAINTAINERS: Add fragment for Xilinx watchdog drivers Srinivas Neeli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).