[PATCH 0/2] Add Realtek Otto WDT support

[PATCH 0/2] Add Realtek Otto WDT support

[Sander Vanheule]

This watchdog timer found on Realtek's Otto MIPS platforms, including the
RTL838x and RTL839x series of ethernet switch SoCs. It has a number of reset
modes (SoC, CPU, software), and can provide pretimeout interrupts.

The timer has two timeout phases. Both phases have a maximum duration of 32
prescaled clock ticks, which is ca. 43s with a clock of 200MHz:
- Phase 1: During this phase, the WDT can be pinged to reset the timeout.
- Phase 2: Starts after phase 1 has timed out, and only serves to give the
  system some time to clean up, or notify others that it's going to reset.
  During this phase, pinging the WDT has no effect, and a reset is unavoidable.

The driver has been tested on a Zyxel GS1900-8 (RTL8380, mainline kernel and
OpenWrt), a Zyxel GS1900-48 (RTL8393, mainline), and a Netgear GS110TPPv1
(RTL8381, mainline).

Sander Vanheule (2):
  dt-bindings: watchdog: Realtek Otto WDT binding
  watchdog: Add Realtek Otto watchdog timer

 .../bindings/watchdog/realtek,otto-wdt.yaml   |  89 ++++
 MAINTAINERS                                   |   7 +
 drivers/watchdog/Kconfig                      |  13 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/realtek_otto_wdt.c           | 411 ++++++++++++++++++
 5 files changed, 521 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
 create mode 100644 drivers/watchdog/realtek_otto_wdt.c

-- 
2.31.1

[PATCH 1/2] dt-bindings: watchdog: Realtek Otto WDT binding

[Sander Vanheule]

Add a binding description for Realtek's watchdog timer as found on
several of their MIPS-based SoCs (codenamed Otto), such as the RTL838x
and RTL839x series of switch SoCs.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 .../bindings/watchdog/realtek,otto-wdt.yaml   | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
new file mode 100644
index 000000000000..b962fd1229ff
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/realtek,otto-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek Otto watchdog timer
+
+maintainers:
+  - Sander Vanheule <sander@svanheule.net>
+
+description: |
+  The timer has two timeout phases. Both phases have a maximum duration of 32
+  prescaled clock ticks, which is ca. 43s with a bus clock of 200MHz. The
+  minimum duration of each phase is one tick. Each phase can trigger an
+  interrupt, although the phase 2 interrupt will occur with the system reset.
+  - Phase 1: During this phase, the WDT can be pinged to reset the timeout.
+  - Phase 2: Starts after phase 1 has timed out, and only serves to give the
+    system some time to clean up, or notify others that it's going to reset.
+    During this phase, pinging the WDT has no effect, and a reset is
+    unavoidable, unless the WDT is disabled.
+
+allOf:
+  - $ref: watchdog.yaml#
+
+properties:
+  compatible:
+    enum:
+      - realtek,rtl8380-wdt
+      - realtek,rtl8390-wdt
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: interrupt specifier for pretimeout
+      - description: interrupt specifier for timeout
+
+  interrupt-names:
+    items:
+      - const: phase1
+      - const: phase2
+
+  realtek,reset-mode:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: |
+      Specify how the system is reset after a timeout. Defaults to "cpu" if
+      left unspecified.
+    oneOf:
+      - description: Reset the entire chip
+        const: soc
+      - description: |
+          Reset the CPU and IPsec engine, but leave other peripherals untouched
+        const: cpu
+      - description: |
+          Reset the execution pointer, but don't actually reset any hardware
+        const: software
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+unevaluatedProperties: false
+
+dependencies:
+  interrupts: [ interrupt-names ]
+
+examples:
+  - |
+    watchdog: watchdog@3150 {
+        compatible = "realtek,rtl8380-wdt";
+        reg = <0x3150 0xc>;
+
+        realtek,reset-mode = "soc";
+
+        clocks = <&lxbus_clock>;
+        timeout-sec = <20>;
+
+        interrupt-parent = <&rtlintc>;
+        interrupt-names = "phase1", "phase2";
+        interrupts = <19>, <18>;
+    };
+
+...
-- 
2.31.1

[PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Sander Vanheule]

Realtek MIPS SoCs (platform name Otto) have a watchdog timer with
pretimeout notifitication support. The WDT can (partially) hard reset,
or soft reset the SoC.

This driver implements all features as described in the devicetree
binding, and also functions as a restart handler. The cpu reset mode is
considered to be a "warm" restart, since this mode does not reset all
peripherals. Being an embedded system though, the "cpu" and "software"
modes will still cause the bootloader to run on restart.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 MAINTAINERS                         |   7 +
 drivers/watchdog/Kconfig            |  13 +
 drivers/watchdog/Makefile           |   1 +
 drivers/watchdog/realtek_otto_wdt.c | 411 ++++++++++++++++++++++++++++
 4 files changed, 432 insertions(+)
 create mode 100644 drivers/watchdog/realtek_otto_wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 62257ffca56a..e2a036c1a64b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15858,6 +15858,13 @@ S:	Maintained
 F:	include/sound/rt*.h
 F:	sound/soc/codecs/rt*
 
+REALTEK OTTO WATCHDOG
+M:	Sander Vanheule <sander@svanheule.net>
+L:	linux-watchdog@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
+F:	driver/watchdog/realtek_otto_wdt.c
+
 REALTEK RTL83xx SMI DSA ROUTER CHIPS
 M:	Linus Walleij <linus.walleij@linaro.org>
 S:	Maintained
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bf59faeb3de1..799dbc14b347 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -954,6 +954,19 @@ config RTD119X_WATCHDOG
 	  Say Y here to include support for the watchdog timer in
 	  Realtek RTD1295 SoCs.
 
+config REALTEK_OTTO_WDT
+	tristate "Realtek Otto MIPS watchdog support"
+	depends on MACH_REALTEK_RTL || COMPILE_TEST
+	select COMMON_CLK
+	select WATCHDOG_CORE
+	default MACH_REALTEK_RTL
+	help
+	  Say Y here to include support for the watchdog timer on
+	  Realtek RTL838x, RTL839x SoCs. On timeout this watchdog
+	  will restart the system.
+
+	  When built as a module this will be called realtek_otto_wdt.
+
 config SPRD_WATCHDOG
 	tristate "Spreadtrum watchdog support"
 	depends on ARCH_SPRD || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 1bd2d6f37c53..a8dccf819163 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -171,6 +171,7 @@ obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
 obj-$(CONFIG_MT7621_WDT) += mt7621_wdt.o
 obj-$(CONFIG_PIC32_WDT) += pic32-wdt.o
 obj-$(CONFIG_PIC32_DMT) += pic32-dmt.o
+obj-$(CONFIG_REALTEK_OTTO_WDT) += realtek_otto_wdt.o
 
 # PARISC Architecture
 
diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
new file mode 100644
index 000000000000..64c9cba6b0b1
--- /dev/null
+++ b/drivers/watchdog/realtek_otto_wdt.c
@@ -0,0 +1,411 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Realtek Otto MIPS platform watchdog
+ *
+ * Watchdog timer that will reset the system after timeout, using the selected
+ * reset mode.
+ *
+ * Counter scaling and timeouts:
+ * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @ 200MHz
+ * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
+ * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
+ *   Generates an interrupt, WDT cannot be stopped after phase 1
+ * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) × PRESCALE × T_0
+ *   Resets the system according to RST_MODE
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/math.h>
+#include <linux/minmax.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define OTTO_WDT_REG_CNTR		0x0
+#define OTTO_WDT_CNTR_PING		BIT(31)
+
+#define OTTO_WDT_REG_INTR		0x4
+#define OTTO_WDT_INTR_PHASE_1		BIT(31)
+#define OTTO_WDT_INTR_PHASE_2		BIT(30)
+
+#define OTTO_WDT_REG_CTRL		0x8
+#define OTTO_WDT_CTRL_ENABLE		BIT(31)
+#define OTTO_WDT_CTRL_PRESCALE		GENMASK(30, 29)
+#define OTTO_WDT_CTRL_PHASE1		GENMASK(26, 22)
+#define OTTO_WDT_CTRL_PHASE2		GENMASK(19, 15)
+#define OTTO_WDT_CTRL_RST_MODE		GENMASK(1, 0)
+#define OTTO_WDT_MODE_SOC		0
+#define OTTO_WDT_MODE_CPU		1
+#define OTTO_WDT_MODE_SOFTWARE		2
+#define OTTO_WDT_CTRL_DEFAULT		OTTO_WDT_MODE_CPU
+
+#define OTTO_WDT_PRESCALE_MAX		3
+
+/*
+ * One higher than the max values contained in PHASE{1,2}, since a value of 0
+ * corresponds to one tick.
+ */
+#define OTTO_WDT_PHASE_TICKS_MAX	32
+
+/*
+ * The maximum reset delay is actually 2×32 ticks, but that would require large
+ * pretimout values for timeouts longer than 32 ticks. Limit the maximum timeout
+ * to 32 + 1 to ensure small pretimeout values can be configured as expected.
+ */
+#define OTTO_WDT_TIMEOUT_TICKS_MAX	(OTTO_WDT_PHASE_TICKS_MAX + 1)
+
+struct otto_wdt_ctrl {
+	struct watchdog_device wdev;
+	struct device *dev;
+	raw_spinlock_t lock;
+	void __iomem *base;
+	struct clk *clk;
+	int irq_phase1;
+	int irq_phase2;
+};
+
+static int otto_wdt_start(struct watchdog_device *wdev)
+{
+	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
+	unsigned long flags;
+	u32 v;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
+	v |= OTTO_WDT_CTRL_ENABLE;
+	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static int otto_wdt_stop(struct watchdog_device *wdev)
+{
+	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
+	unsigned long flags;
+	u32 v;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
+	v &= ~OTTO_WDT_CTRL_ENABLE;
+	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return 0;
+}
+
+static int otto_wdt_ping(struct watchdog_device *wdev)
+{
+	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
+
+	iowrite32(OTTO_WDT_CNTR_PING, ctrl->base + OTTO_WDT_REG_CNTR);
+
+	return 0;
+}
+
+static int otto_wdt_tick_ms(struct otto_wdt_ctrl *ctrl, int prescale)
+{
+	unsigned int rate_khz = clk_get_rate(ctrl->clk) / 1000;
+
+	if (!rate_khz)
+		return 0;
+
+	return DIV_ROUND_CLOSEST(1 << (25 + prescale), rate_khz);
+}
+
+/*
+ * The timer asserts the PHASE1/PHASE2 IRQs when the number of ticks exceeds
+ * the value stored in those fields. This means the timer will run for at least
+ * one tick, so small values need to be clamped to correctly reflect the timeout.
+ */
+static inline unsigned int div_round_ticks(unsigned int val, unsigned int tick_duration,
+		unsigned int min_ticks)
+{
+	return max(min_ticks, DIV_ROUND_CLOSEST(val, tick_duration));
+}
+
+static int otto_wdt_determine_timeouts(struct otto_wdt_ctrl *ctrl, unsigned int timeout,
+		unsigned int pretimeout)
+{
+	unsigned int pretimeout_ms = pretimeout * 1000;
+	unsigned int timeout_ms = timeout * 1000;
+	unsigned int prescale_next = 0;
+	unsigned int phase1_ticks;
+	unsigned int phase2_ticks;
+	unsigned int total_ticks;
+	unsigned int prescale;
+	unsigned int tick_ms;
+	u32 v;
+
+	do {
+		prescale = prescale_next;
+		if (prescale > OTTO_WDT_PRESCALE_MAX)
+			return -EINVAL;
+
+		tick_ms = otto_wdt_tick_ms(ctrl, prescale);
+		total_ticks = div_round_ticks(timeout_ms, tick_ms, 2);
+		phase2_ticks = div_round_ticks(pretimeout_ms, tick_ms, 1);
+		phase1_ticks = total_ticks - phase2_ticks;
+
+		prescale_next++;
+	} while (phase1_ticks > OTTO_WDT_PHASE_TICKS_MAX
+		|| phase2_ticks > OTTO_WDT_PHASE_TICKS_MAX);
+
+	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
+
+	v &= ~(OTTO_WDT_CTRL_PRESCALE | OTTO_WDT_CTRL_PHASE1 | OTTO_WDT_CTRL_PHASE2);
+	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE1, phase1_ticks - 1);
+	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
+	v |= FIELD_PREP(OTTO_WDT_CTRL_PRESCALE, prescale);
+
+	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
+
+	timeout_ms = total_ticks * tick_ms;
+	ctrl->wdev.timeout = DIV_ROUND_CLOSEST(timeout_ms, 1000);
+
+	pretimeout_ms = phase2_ticks * tick_ms;
+	ctrl->wdev.pretimeout = DIV_ROUND_CLOSEST(pretimeout_ms, 1000);
+
+	return 0;
+}
+
+static int otto_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
+{
+	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
+	unsigned long flags;
+	unsigned int ret;
+
+	if (watchdog_timeout_invalid(wdev, val))
+		return -EINVAL;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	ret = otto_wdt_determine_timeouts(ctrl, val, min(wdev->pretimeout, val - 1));
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return ret;
+}
+
+static int otto_wdt_set_pretimeout(struct watchdog_device *wdev, unsigned int val)
+{
+	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
+	unsigned long flags;
+	unsigned int ret;
+
+	if (watchdog_pretimeout_invalid(wdev, val))
+		return -EINVAL;
+
+	raw_spin_lock_irqsave(&ctrl->lock, flags);
+	ret = otto_wdt_determine_timeouts(ctrl, wdev->timeout, val);
+	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
+
+	return ret;
+}
+
+static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_mode,
+		void *data)
+{
+	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
+	u32 reset_mode;
+	u32 v;
+
+	devm_free_irq(ctrl->dev, ctrl->irq_phase1, ctrl);
+
+	switch (reboot_mode) {
+	case REBOOT_SOFT:
+		reset_mode = OTTO_WDT_MODE_SOFTWARE;
+		break;
+	case REBOOT_WARM:
+		reset_mode = OTTO_WDT_MODE_CPU;
+		break;
+	default:
+		reset_mode = OTTO_WDT_MODE_SOC;
+		break;
+	}
+
+	/* Configure for shortest timeout and wait for reset to occur */
+	v = FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, reset_mode) | OTTO_WDT_CTRL_ENABLE;
+	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
+
+	mdelay(3 * otto_wdt_tick_ms(ctrl, 0));
+
+	return 0;
+}
+
+static irqreturn_t otto_wdt_phase1_isr(int irq, void *dev_id)
+{
+	struct otto_wdt_ctrl *ctrl = dev_id;
+
+	iowrite32(OTTO_WDT_INTR_PHASE_1, ctrl->base + OTTO_WDT_REG_INTR);
+	dev_crit(ctrl->dev, "phase 1 timeout\n");
+	watchdog_notify_pretimeout(&ctrl->wdev);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t otto_wdt_phase2_isr(int irq, void *dev_id)
+{
+	struct otto_wdt_ctrl *ctrl = dev_id;
+
+	iowrite32(OTTO_WDT_INTR_PHASE_2, ctrl->base + OTTO_WDT_REG_INTR);
+	dev_crit(ctrl->dev, "phase 2 timeout\n");
+	emergency_restart();
+
+	return IRQ_HANDLED;
+}
+
+static const struct watchdog_ops otto_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = otto_wdt_start,
+	.stop = otto_wdt_stop,
+	.ping = otto_wdt_ping,
+	.set_timeout = otto_wdt_set_timeout,
+	.set_pretimeout = otto_wdt_set_pretimeout,
+	.restart = otto_wdt_restart,
+};
+
+static const struct watchdog_info otto_wdt_info = {
+	.identity = "Realtek Otto watchdog timer",
+	.options = WDIOF_KEEPALIVEPING |
+		WDIOF_MAGICCLOSE |
+		WDIOF_SETTIMEOUT |
+		WDIOF_PRETIMEOUT,
+};
+
+static int otto_wdt_probe_reset_mode(struct otto_wdt_ctrl *ctrl)
+{
+	static const char *mode_property = "realtek,reset-mode";
+	const struct fwnode_handle *node = ctrl->dev->fwnode;
+	int mode_count;
+	u32 mode;
+	u32 v;
+
+	if (!node)
+		return -ENXIO;
+
+	mode_count = fwnode_property_string_array_count(node, mode_property);
+	if (mode_count < 0)
+		return mode_count;
+	else if (mode_count == 0)
+		return 0;
+	else if (mode_count != 1)
+		return -EINVAL;
+
+	if (fwnode_property_match_string(node, mode_property, "soc") == 0)
+		mode = OTTO_WDT_MODE_SOC;
+	else if (fwnode_property_match_string(node, mode_property, "cpu") == 0)
+		mode = OTTO_WDT_MODE_CPU;
+	else if (fwnode_property_match_string(node, mode_property, "software") == 0)
+		mode = OTTO_WDT_MODE_SOFTWARE;
+	else
+		return -EINVAL;
+
+	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
+	v &= ~OTTO_WDT_CTRL_RST_MODE;
+	v |= FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, mode);
+	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
+
+	return 0;
+}
+
+static int otto_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct otto_wdt_ctrl *ctrl;
+	unsigned int max_tick_ms;
+	int ret;
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	ctrl->dev = dev;
+	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ctrl->base))
+		return PTR_ERR(ctrl->base);
+
+	raw_spin_lock_init(&ctrl->lock);
+
+	/* Clear any old interrupts and reset initial state */
+	iowrite32(OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2,
+			ctrl->base + OTTO_WDT_REG_INTR);
+	iowrite32(OTTO_WDT_CTRL_DEFAULT, ctrl->base + OTTO_WDT_REG_CTRL);
+
+	ctrl->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(ctrl->clk))
+		return dev_err_probe(dev, PTR_ERR(ctrl->clk), "Failed to get clock\n");
+
+	ctrl->irq_phase1 = platform_get_irq_byname(pdev, "phase1");
+	if (ctrl->irq_phase1 < 0)
+		return dev_err_probe(dev, ctrl->irq_phase1, "phase1 IRQ not found\n");
+
+	ret = devm_request_irq(dev, ctrl->irq_phase1, otto_wdt_phase1_isr, 0,
+			"realtek-otto-wdt", ctrl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get IRQ for phase1\n");
+
+	ctrl->irq_phase2 = platform_get_irq_byname(pdev, "phase2");
+	if (ctrl->irq_phase2 < 0)
+		return dev_err_probe(dev, ctrl->irq_phase2, "phase2 IRQ not found\n");
+
+	ret = devm_request_irq(dev, ctrl->irq_phase2, otto_wdt_phase2_isr, 0,
+			"realtek-otto-wdt", ctrl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get IRQ for phase2\n");
+
+	ret = otto_wdt_probe_reset_mode(ctrl);
+	if (ret)
+		return dev_err_probe(dev, ret, "Invalid reset mode specified\n");
+
+	ctrl->wdev.parent = dev;
+	ctrl->wdev.info = &otto_wdt_info;
+	ctrl->wdev.ops = &otto_wdt_ops;
+
+	/*
+	 * Since pretimeout cannot be disabled, min_timeout is twice the
+	 * subsystem resolution. max_timeout is 44s at a bus clock of 200MHz.
+	 */
+	ctrl->wdev.min_timeout = 2;
+	max_tick_ms = otto_wdt_tick_ms(ctrl, OTTO_WDT_PRESCALE_MAX);
+	ctrl->wdev.max_timeout =
+		DIV_ROUND_CLOSEST(max_tick_ms * OTTO_WDT_TIMEOUT_TICKS_MAX, 1000);
+	ctrl->wdev.timeout = min(30U, ctrl->wdev.max_timeout);
+
+	watchdog_set_drvdata(&ctrl->wdev, ctrl);
+	watchdog_init_timeout(&ctrl->wdev, 0, dev);
+	watchdog_stop_on_reboot(&ctrl->wdev);
+	watchdog_set_restart_priority(&ctrl->wdev, 128);
+
+	ret = otto_wdt_determine_timeouts(ctrl, ctrl->wdev.timeout, 1);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set timeout\n");
+
+	return devm_watchdog_register_device(dev, &ctrl->wdev);
+}
+
+static const struct of_device_id otto_wdt_ids[] = {
+	{ .compatible = "realtek,rtl8380-wdt" },
+	{ .compatible = "realtek,rtl8390-wdt" },
+	{ }
+};
+
+static struct platform_driver otto_wdt_driver = {
+	.probe = otto_wdt_probe,
+	.driver = {
+		.name = "realtek-otto-watchdog",
+		.of_match_table	= otto_wdt_ids,
+	},
+};
+module_platform_driver(otto_wdt_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_DESCRIPTION("Realtek Otto watchdog timer driver");
-- 
2.31.1

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Guenter Roeck]

On Wed, Oct 13, 2021 at 03:29:00PM +0200, Sander Vanheule wrote:
> Realtek MIPS SoCs (platform name Otto) have a watchdog timer with
> pretimeout notifitication support. The WDT can (partially) hard reset,
> or soft reset the SoC.
> 
> This driver implements all features as described in the devicetree
> binding, and also functions as a restart handler. The cpu reset mode is
> considered to be a "warm" restart, since this mode does not reset all
> peripherals. Being an embedded system though, the "cpu" and "software"
> modes will still cause the bootloader to run on restart.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  MAINTAINERS                         |   7 +
>  drivers/watchdog/Kconfig            |  13 +
>  drivers/watchdog/Makefile           |   1 +
>  drivers/watchdog/realtek_otto_wdt.c | 411 ++++++++++++++++++++++++++++
>  4 files changed, 432 insertions(+)
>  create mode 100644 drivers/watchdog/realtek_otto_wdt.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 62257ffca56a..e2a036c1a64b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15858,6 +15858,13 @@ S:	Maintained
>  F:	include/sound/rt*.h
>  F:	sound/soc/codecs/rt*
>  
> +REALTEK OTTO WATCHDOG
> +M:	Sander Vanheule <sander@svanheule.net>
> +L:	linux-watchdog@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
> +F:	driver/watchdog/realtek_otto_wdt.c
> +
>  REALTEK RTL83xx SMI DSA ROUTER CHIPS
>  M:	Linus Walleij <linus.walleij@linaro.org>
>  S:	Maintained
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index bf59faeb3de1..799dbc14b347 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -954,6 +954,19 @@ config RTD119X_WATCHDOG
>  	  Say Y here to include support for the watchdog timer in
>  	  Realtek RTD1295 SoCs.
>  
> +config REALTEK_OTTO_WDT
> +	tristate "Realtek Otto MIPS watchdog support"
> +	depends on MACH_REALTEK_RTL || COMPILE_TEST
> +	select COMMON_CLK
> +	select WATCHDOG_CORE
> +	default MACH_REALTEK_RTL
> +	help
> +	  Say Y here to include support for the watchdog timer on
> +	  Realtek RTL838x, RTL839x SoCs. On timeout this watchdog
> +	  will restart the system.
> +
> +	  When built as a module this will be called realtek_otto_wdt.
> +
>  config SPRD_WATCHDOG
>  	tristate "Spreadtrum watchdog support"
>  	depends on ARCH_SPRD || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 1bd2d6f37c53..a8dccf819163 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -171,6 +171,7 @@ obj-$(CONFIG_IMGPDC_WDT) += imgpdc_wdt.o
>  obj-$(CONFIG_MT7621_WDT) += mt7621_wdt.o
>  obj-$(CONFIG_PIC32_WDT) += pic32-wdt.o
>  obj-$(CONFIG_PIC32_DMT) += pic32-dmt.o
> +obj-$(CONFIG_REALTEK_OTTO_WDT) += realtek_otto_wdt.o
>  
>  # PARISC Architecture
>  
> diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
> new file mode 100644
> index 000000000000..64c9cba6b0b1
> --- /dev/null
> +++ b/drivers/watchdog/realtek_otto_wdt.c
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Realtek Otto MIPS platform watchdog
> + *
> + * Watchdog timer that will reset the system after timeout, using the selected
> + * reset mode.
> + *
> + * Counter scaling and timeouts:
> + * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @ 200MHz
> + * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
> + * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
> + *   Generates an interrupt, WDT cannot be stopped after phase 1
> + * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) × PRESCALE × T_0
> + *   Resets the system according to RST_MODE

Why is there a phase2 interrupt if phase2 resets the chip ?

> + */
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/reboot.h>
> +#include <linux/watchdog.h>
> +
> +#define OTTO_WDT_REG_CNTR		0x0
> +#define OTTO_WDT_CNTR_PING		BIT(31)
> +
> +#define OTTO_WDT_REG_INTR		0x4
> +#define OTTO_WDT_INTR_PHASE_1		BIT(31)
> +#define OTTO_WDT_INTR_PHASE_2		BIT(30)
> +
> +#define OTTO_WDT_REG_CTRL		0x8
> +#define OTTO_WDT_CTRL_ENABLE		BIT(31)
> +#define OTTO_WDT_CTRL_PRESCALE		GENMASK(30, 29)
> +#define OTTO_WDT_CTRL_PHASE1		GENMASK(26, 22)
> +#define OTTO_WDT_CTRL_PHASE2		GENMASK(19, 15)
> +#define OTTO_WDT_CTRL_RST_MODE		GENMASK(1, 0)
> +#define OTTO_WDT_MODE_SOC		0
> +#define OTTO_WDT_MODE_CPU		1
> +#define OTTO_WDT_MODE_SOFTWARE		2
> +#define OTTO_WDT_CTRL_DEFAULT		OTTO_WDT_MODE_CPU
> +
> +#define OTTO_WDT_PRESCALE_MAX		3
> +
> +/*
> + * One higher than the max values contained in PHASE{1,2}, since a value of 0
> + * corresponds to one tick.
> + */
> +#define OTTO_WDT_PHASE_TICKS_MAX	32
> +
> +/*
> + * The maximum reset delay is actually 2×32 ticks, but that would require large
> + * pretimout values for timeouts longer than 32 ticks. Limit the maximum timeout
> + * to 32 + 1 to ensure small pretimeout values can be configured as expected.
> + */
> +#define OTTO_WDT_TIMEOUT_TICKS_MAX	(OTTO_WDT_PHASE_TICKS_MAX + 1)
> +
> +struct otto_wdt_ctrl {
> +	struct watchdog_device wdev;
> +	struct device *dev;
> +	raw_spinlock_t lock;
> +	void __iomem *base;
> +	struct clk *clk;
> +	int irq_phase1;
> +	int irq_phase2;
> +};
> +
> +static int otto_wdt_start(struct watchdog_device *wdev)
> +{
> +	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> +	unsigned long flags;
> +	u32 v;
> +
> +	raw_spin_lock_irqsave(&ctrl->lock, flags);
> +	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> +	v |= OTTO_WDT_CTRL_ENABLE;
> +	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +	raw_spin_unlock_irqrestore(&ctrl->lock, flags);

Is it really necessary to disable interrupts for those operations ?

> +
> +	return 0;
> +}
> +
> +static int otto_wdt_stop(struct watchdog_device *wdev)
> +{
> +	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> +	unsigned long flags;
> +	u32 v;
> +
> +	raw_spin_lock_irqsave(&ctrl->lock, flags);
> +	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> +	v &= ~OTTO_WDT_CTRL_ENABLE;
> +	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int otto_wdt_ping(struct watchdog_device *wdev)
> +{
> +	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> +
> +	iowrite32(OTTO_WDT_CNTR_PING, ctrl->base + OTTO_WDT_REG_CNTR);
> +
> +	return 0;
> +}
> +
> +static int otto_wdt_tick_ms(struct otto_wdt_ctrl *ctrl, int prescale)
> +{
> +	unsigned int rate_khz = clk_get_rate(ctrl->clk) / 1000;
> +
> +	if (!rate_khz)
> +		return 0;
> +
> +	return DIV_ROUND_CLOSEST(1 << (25 + prescale), rate_khz);
> +}
> +
> +/*
> + * The timer asserts the PHASE1/PHASE2 IRQs when the number of ticks exceeds
> + * the value stored in those fields. This means the timer will run for at least
> + * one tick, so small values need to be clamped to correctly reflect the timeout.
> + */
> +static inline unsigned int div_round_ticks(unsigned int val, unsigned int tick_duration,
> +		unsigned int min_ticks)
> +{
> +	return max(min_ticks, DIV_ROUND_CLOSEST(val, tick_duration));

Are you sure that DIV_ROUND_CLOSEST is appropriate in those calculations
(instead of DIV_ROUND_UP or DIV_ROUND_DOWN) ?

> +}
> +
> +static int otto_wdt_determine_timeouts(struct otto_wdt_ctrl *ctrl, unsigned int timeout,
> +		unsigned int pretimeout)
> +{
> +	unsigned int pretimeout_ms = pretimeout * 1000;
> +	unsigned int timeout_ms = timeout * 1000;
> +	unsigned int prescale_next = 0;
> +	unsigned int phase1_ticks;
> +	unsigned int phase2_ticks;
> +	unsigned int total_ticks;
> +	unsigned int prescale;
> +	unsigned int tick_ms;
> +	u32 v;
> +
> +	do {
> +		prescale = prescale_next;
> +		if (prescale > OTTO_WDT_PRESCALE_MAX)
> +			return -EINVAL;
> +
> +		tick_ms = otto_wdt_tick_ms(ctrl, prescale);
> +		total_ticks = div_round_ticks(timeout_ms, tick_ms, 2);
> +		phase2_ticks = div_round_ticks(pretimeout_ms, tick_ms, 1);
> +		phase1_ticks = total_ticks - phase2_ticks;
> +
> +		prescale_next++;
> +	} while (phase1_ticks > OTTO_WDT_PHASE_TICKS_MAX
> +		|| phase2_ticks > OTTO_WDT_PHASE_TICKS_MAX);
> +
> +	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> +
> +	v &= ~(OTTO_WDT_CTRL_PRESCALE | OTTO_WDT_CTRL_PHASE1 | OTTO_WDT_CTRL_PHASE2);
> +	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE1, phase1_ticks - 1);
> +	v |= FIELD_PREP(OTTO_WDT_CTRL_PHASE2, phase2_ticks - 1);
> +	v |= FIELD_PREP(OTTO_WDT_CTRL_PRESCALE, prescale);
> +
> +	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +
> +	timeout_ms = total_ticks * tick_ms;
> +	ctrl->wdev.timeout = DIV_ROUND_CLOSEST(timeout_ms, 1000);
> +

That means the actual timeout (and pretimeout) can be slightly larger
than the real timeout. Is this really what you want ?

> +	pretimeout_ms = phase2_ticks * tick_ms;
> +	ctrl->wdev.pretimeout = DIV_ROUND_CLOSEST(pretimeout_ms, 1000);
> +
> +	return 0;
> +}
> +
> +static int otto_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
> +{
> +	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> +	unsigned long flags;
> +	unsigned int ret;
> +
> +	if (watchdog_timeout_invalid(wdev, val))
> +		return -EINVAL;

This is not supposed to happen because the calling code already performs
range checks.

> +
> +	raw_spin_lock_irqsave(&ctrl->lock, flags);
> +	ret = otto_wdt_determine_timeouts(ctrl, val, min(wdev->pretimeout, val - 1));
> +	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int otto_wdt_set_pretimeout(struct watchdog_device *wdev, unsigned int val)
> +{
> +	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> +	unsigned long flags;
> +	unsigned int ret;
> +
> +	if (watchdog_pretimeout_invalid(wdev, val))
> +		return -EINVAL;
> +
> +	raw_spin_lock_irqsave(&ctrl->lock, flags);
> +	ret = otto_wdt_determine_timeouts(ctrl, wdev->timeout, val);
> +	raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_mode,
> +		void *data)
> +{
> +	struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> +	u32 reset_mode;
> +	u32 v;
> +
> +	devm_free_irq(ctrl->dev, ctrl->irq_phase1, ctrl);
> +

Why is this needed (instead of, say, disabling the interrupt) ?

> +	switch (reboot_mode) {
> +	case REBOOT_SOFT:
> +		reset_mode = OTTO_WDT_MODE_SOFTWARE;
> +		break;
> +	case REBOOT_WARM:
> +		reset_mode = OTTO_WDT_MODE_CPU;
> +		break;
> +	default:
> +		reset_mode = OTTO_WDT_MODE_SOC;
> +		break;
> +	}
> +
> +	/* Configure for shortest timeout and wait for reset to occur */
> +	v = FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, reset_mode) | OTTO_WDT_CTRL_ENABLE;
> +	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +
> +	mdelay(3 * otto_wdt_tick_ms(ctrl, 0));
> +
> +	return 0;
> +}
> +
> +static irqreturn_t otto_wdt_phase1_isr(int irq, void *dev_id)
> +{
> +	struct otto_wdt_ctrl *ctrl = dev_id;
> +
> +	iowrite32(OTTO_WDT_INTR_PHASE_1, ctrl->base + OTTO_WDT_REG_INTR);
> +	dev_crit(ctrl->dev, "phase 1 timeout\n");
> +	watchdog_notify_pretimeout(&ctrl->wdev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t otto_wdt_phase2_isr(int irq, void *dev_id)
> +{
> +	struct otto_wdt_ctrl *ctrl = dev_id;
> +
> +	iowrite32(OTTO_WDT_INTR_PHASE_2, ctrl->base + OTTO_WDT_REG_INTR);
> +	dev_crit(ctrl->dev, "phase 2 timeout\n");
> +	emergency_restart();
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct watchdog_ops otto_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = otto_wdt_start,
> +	.stop = otto_wdt_stop,
> +	.ping = otto_wdt_ping,
> +	.set_timeout = otto_wdt_set_timeout,
> +	.set_pretimeout = otto_wdt_set_pretimeout,
> +	.restart = otto_wdt_restart,
> +};
> +
> +static const struct watchdog_info otto_wdt_info = {
> +	.identity = "Realtek Otto watchdog timer",
> +	.options = WDIOF_KEEPALIVEPING |
> +		WDIOF_MAGICCLOSE |
> +		WDIOF_SETTIMEOUT |
> +		WDIOF_PRETIMEOUT,
> +};
> +
> +static int otto_wdt_probe_reset_mode(struct otto_wdt_ctrl *ctrl)
> +{
> +	static const char *mode_property = "realtek,reset-mode";
> +	const struct fwnode_handle *node = ctrl->dev->fwnode;
> +	int mode_count;
> +	u32 mode;
> +	u32 v;
> +
> +	if (!node)
> +		return -ENXIO;
> +
> +	mode_count = fwnode_property_string_array_count(node, mode_property);
> +	if (mode_count < 0)
> +		return mode_count;
> +	else if (mode_count == 0)
> +		return 0;
> +	else if (mode_count != 1)
> +		return -EINVAL;
> +
> +	if (fwnode_property_match_string(node, mode_property, "soc") == 0)
> +		mode = OTTO_WDT_MODE_SOC;
> +	else if (fwnode_property_match_string(node, mode_property, "cpu") == 0)
> +		mode = OTTO_WDT_MODE_CPU;
> +	else if (fwnode_property_match_string(node, mode_property, "software") == 0)
> +		mode = OTTO_WDT_MODE_SOFTWARE;
> +	else
> +		return -EINVAL;
> +
> +	v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> +	v &= ~OTTO_WDT_CTRL_RST_MODE;
> +	v |= FIELD_PREP(OTTO_WDT_CTRL_RST_MODE, mode);
> +	iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> +
> +	return 0;
> +}
> +
> +static int otto_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct otto_wdt_ctrl *ctrl;
> +	unsigned int max_tick_ms;
> +	int ret;
> +
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	ctrl->dev = dev;
> +	ctrl->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ctrl->base))
> +		return PTR_ERR(ctrl->base);
> +
> +	raw_spin_lock_init(&ctrl->lock);
> +
> +	/* Clear any old interrupts and reset initial state */
> +	iowrite32(OTTO_WDT_INTR_PHASE_1 | OTTO_WDT_INTR_PHASE_2,
> +			ctrl->base + OTTO_WDT_REG_INTR);
> +	iowrite32(OTTO_WDT_CTRL_DEFAULT, ctrl->base + OTTO_WDT_REG_CTRL);
> +
> +	ctrl->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(ctrl->clk))
> +		return dev_err_probe(dev, PTR_ERR(ctrl->clk), "Failed to get clock\n");
> +
> +	ctrl->irq_phase1 = platform_get_irq_byname(pdev, "phase1");
> +	if (ctrl->irq_phase1 < 0)
> +		return dev_err_probe(dev, ctrl->irq_phase1, "phase1 IRQ not found\n");
> +
> +	ret = devm_request_irq(dev, ctrl->irq_phase1, otto_wdt_phase1_isr, 0,
> +			"realtek-otto-wdt", ctrl);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get IRQ for phase1\n");
> +
> +	ctrl->irq_phase2 = platform_get_irq_byname(pdev, "phase2");
> +	if (ctrl->irq_phase2 < 0)
> +		return dev_err_probe(dev, ctrl->irq_phase2, "phase2 IRQ not found\n");
> +
> +	ret = devm_request_irq(dev, ctrl->irq_phase2, otto_wdt_phase2_isr, 0,
> +			"realtek-otto-wdt", ctrl);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get IRQ for phase2\n");
> +
> +	ret = otto_wdt_probe_reset_mode(ctrl);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Invalid reset mode specified\n");
> +
> +	ctrl->wdev.parent = dev;
> +	ctrl->wdev.info = &otto_wdt_info;
> +	ctrl->wdev.ops = &otto_wdt_ops;
> +
> +	/*
> +	 * Since pretimeout cannot be disabled, min_timeout is twice the
> +	 * subsystem resolution. max_timeout is 44s at a bus clock of 200MHz.
> +	 */
> +	ctrl->wdev.min_timeout = 2;
> +	max_tick_ms = otto_wdt_tick_ms(ctrl, OTTO_WDT_PRESCALE_MAX);
> +	ctrl->wdev.max_timeout =
> +		DIV_ROUND_CLOSEST(max_tick_ms * OTTO_WDT_TIMEOUT_TICKS_MAX, 1000);

Any reason for using max_timeout instead of max_hw_heartbeat_ms ?

> +	ctrl->wdev.timeout = min(30U, ctrl->wdev.max_timeout);
> +
> +	watchdog_set_drvdata(&ctrl->wdev, ctrl);
> +	watchdog_init_timeout(&ctrl->wdev, 0, dev);
> +	watchdog_stop_on_reboot(&ctrl->wdev);
> +	watchdog_set_restart_priority(&ctrl->wdev, 128);
> +
> +	ret = otto_wdt_determine_timeouts(ctrl, ctrl->wdev.timeout, 1);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set timeout\n");
> +
> +	return devm_watchdog_register_device(dev, &ctrl->wdev);
> +}
> +
> +static const struct of_device_id otto_wdt_ids[] = {
> +	{ .compatible = "realtek,rtl8380-wdt" },
> +	{ .compatible = "realtek,rtl8390-wdt" },
> +	{ }
> +};
> +
> +static struct platform_driver otto_wdt_driver = {
> +	.probe = otto_wdt_probe,
> +	.driver = {
> +		.name = "realtek-otto-watchdog",
> +		.of_match_table	= otto_wdt_ids,
> +	},
> +};
> +module_platform_driver(otto_wdt_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
> +MODULE_DESCRIPTION("Realtek Otto watchdog timer driver");
> -- 
> 2.31.1
> 

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Sander Vanheule]

On Wed, 2021-10-13 at 11:48 -0700, Guenter Roeck wrote:
> On Wed, Oct 13, 2021 at 03:29:00PM +0200, Sander Vanheule wrote:
[...]

> > 
> > diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
> > new file mode 100644
> > index 000000000000..64c9cba6b0b1
> > --- /dev/null
> > +++ b/drivers/watchdog/realtek_otto_wdt.c
> > @@ -0,0 +1,411 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +/*
> > + * Realtek Otto MIPS platform watchdog
> > + *
> > + * Watchdog timer that will reset the system after timeout, using the selected
> > + * reset mode.
> > + *
> > + * Counter scaling and timeouts:
> > + * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @ 200MHz
> > + * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
> > + * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
> > + *   Generates an interrupt, WDT cannot be stopped after phase 1
> > + * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) × PRESCALE × T_0
> > + *   Resets the system according to RST_MODE
> 
> Why is there a phase2 interrupt if phase2 resets the chip ?
> 

The SoC's reset controller has an interrupt line for phase2, even though then it then the
WDT also resets the system. I don't have any documentation about this peripheral; just
some vendor code and there the phase2 interrupt isn't enabled. I mainly added it here for
completeness.

One thing to note is that after CPU or software reset (not SoC reset) the phase2 flag in
OTTO_WDT_REG_INTR will be set. That's why I always clear it in otto_wdt_probe(), because
otherwise enabling the interrupt line would trigger otto_wdt_phase2_isr(). On warm
restarts this bit could be used to determine if there was a WDT timeout, but not if the
WDT is configured for cold restarts (i.e. full SoC reset).

> 
[...]
> > +
> > +       raw_spin_lock_irqsave(&ctrl->lock, flags);
> > +       v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> > +       v |= OTTO_WDT_CTRL_ENABLE;
> > +       iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> > +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> 
> Is it really necessary to disable interrupts for those operations ?

The ISR routines only use REG_INTR, which isn't modified anywhere else (outside of probing
the device). I will replace these with raw_spin_{lock,unlock} throughout.

[...]
> > +/*
> > + * The timer asserts the PHASE1/PHASE2 IRQs when the number of ticks exceeds
> > + * the value stored in those fields. This means the timer will run for at least
> > + * one tick, so small values need to be clamped to correctly reflect the timeout.
> > + */
> > +static inline unsigned int div_round_ticks(unsigned int val, unsigned int
> > tick_duration,
> > +               unsigned int min_ticks)
> > +{
> > +       return max(min_ticks, DIV_ROUND_CLOSEST(val, tick_duration));
> 
> Are you sure that DIV_ROUND_CLOSEST is appropriate in those calculations
> (instead of DIV_ROUND_UP or DIV_ROUND_DOWN) ?
> 
[...]

> > +
> > +       timeout_ms = total_ticks * tick_ms;
> > +       ctrl->wdev.timeout = DIV_ROUND_CLOSEST(timeout_ms, 1000);
> > +
> 
> That means the actual timeout (and pretimeout) can be slightly larger
> than the real timeout. Is this really what you want ?

Is it a problem if the WDT times out later than specified by watchdog_device.(pre)timeout?
I can see that premature timeouts would be an issue, but I don't suppose it's problematic
if the WDT is pinged (slightly) sooner than actually required?

The coarsest ticks are 1342 ms, so it is not always possible to provide the requested
(pre)timeout value, independent of the rounding scheme. Although I think it should be
possible to replace timeout rounding by DIV_ROUND_UP (of total_ticks_ms), and pretimeout
rounding by DIV_ROUND_DOWN (of phase2_ticks_ms), and keep stable timeouts when alternating
between set_timeout/set_pretimeout.

> 
> > +       pretimeout_ms = phase2_ticks * tick_ms;
> > +       ctrl->wdev.pretimeout = DIV_ROUND_CLOSEST(pretimeout_ms, 1000);
> > +
> > +       return 0;
> > +}
> > +
> > +static int otto_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
> > +{
> > +       struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> > +       unsigned long flags;
> > +       unsigned int ret;
> > +
> > +       if (watchdog_timeout_invalid(wdev, val))
> > +               return -EINVAL;
> 
> This is not supposed to happen because the calling code already performs
> range checks.

Right, I will drop the redundant check here and in set_pretimeout.

> 
[...]
> > +static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_mode,
> > +               void *data)
> > +{
> > +       struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> > +       u32 reset_mode;
> > +       u32 v;
> > +
> > +       devm_free_irq(ctrl->dev, ctrl->irq_phase1, ctrl);
> > +
> 
> Why is this needed (instead of, say, disabling the interrupt) ?

Disabling the interrupt should actually be enough. I'll replace the devm_free_irq() with
disable_irq(). Somehow I didn't find disable_irq(), even though that was what I was
looking for...

[...]
> > +
> > +       /*
> > +        * Since pretimeout cannot be disabled, min_timeout is twice the
> > +        * subsystem resolution. max_timeout is 44s at a bus clock of 200MHz.
> > +        */
> > +       ctrl->wdev.min_timeout = 2;
> > +       max_tick_ms = otto_wdt_tick_ms(ctrl, OTTO_WDT_PRESCALE_MAX);
> > +       ctrl->wdev.max_timeout =
> > +               DIV_ROUND_CLOSEST(max_tick_ms * OTTO_WDT_TIMEOUT_TICKS_MAX, 1000);
> 
> Any reason for using max_timeout instead of max_hw_heartbeat_ms ?

I must have missed this when I was looking at watchdog_device. Makes sense to use
max_hw_heartbeat_ms since that reflects the actual value more accurately.

> 

Thanks for the feedback!

Best,
Sander

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Guenter Roeck]

On 10/13/21 12:46 PM, Sander Vanheule wrote:
> On Wed, 2021-10-13 at 11:48 -0700, Guenter Roeck wrote:
>> On Wed, Oct 13, 2021 at 03:29:00PM +0200, Sander Vanheule wrote:
> [...]
> 
>>>
>>> diff --git a/drivers/watchdog/realtek_otto_wdt.c b/drivers/watchdog/realtek_otto_wdt.c
>>> new file mode 100644
>>> index 000000000000..64c9cba6b0b1
>>> --- /dev/null
>>> +++ b/drivers/watchdog/realtek_otto_wdt.c
>>> @@ -0,0 +1,411 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +/*
>>> + * Realtek Otto MIPS platform watchdog
>>> + *
>>> + * Watchdog timer that will reset the system after timeout, using the selected
>>> + * reset mode.
>>> + *
>>> + * Counter scaling and timeouts:
>>> + * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @ 200MHz
>>> + * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
>>> + * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
>>> + *   Generates an interrupt, WDT cannot be stopped after phase 1
>>> + * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) × PRESCALE × T_0
>>> + *   Resets the system according to RST_MODE
>>
>> Why is there a phase2 interrupt if phase2 resets the chip ?
>>
> 
> The SoC's reset controller has an interrupt line for phase2, even though then it then the
> WDT also resets the system. I don't have any documentation about this peripheral; just
> some vendor code and there the phase2 interrupt isn't enabled. I mainly added it here for
> completeness.
> 

It seems pointless to mandate an interrupt just for completeness.

> One thing to note is that after CPU or software reset (not SoC reset) the phase2 flag in
> OTTO_WDT_REG_INTR will be set. That's why I always clear it in otto_wdt_probe(), because
> otherwise enabling the interrupt line would trigger otto_wdt_phase2_isr(). On warm
> restarts this bit could be used to determine if there was a WDT timeout, but not if the
> WDT is configured for cold restarts (i.e. full SoC reset).
> 
>>
> [...]
>>> +
>>> +       raw_spin_lock_irqsave(&ctrl->lock, flags);
>>> +       v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
>>> +       v |= OTTO_WDT_CTRL_ENABLE;
>>> +       iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
>>> +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
>>
>> Is it really necessary to disable interrupts for those operations ?
> 
> The ISR routines only use REG_INTR, which isn't modified anywhere else (outside of probing
> the device). I will replace these with raw_spin_{lock,unlock} throughout.
> 

In that case you should not need any locks at all since the watchdog core ensures
that the device is opened only once (and thus only one entity can enable or disable
the watchdog).

> [...]
>>> +/*
>>> + * The timer asserts the PHASE1/PHASE2 IRQs when the number of ticks exceeds
>>> + * the value stored in those fields. This means the timer will run for at least
>>> + * one tick, so small values need to be clamped to correctly reflect the timeout.
>>> + */
>>> +static inline unsigned int div_round_ticks(unsigned int val, unsigned int
>>> tick_duration,
>>> +               unsigned int min_ticks)
>>> +{
>>> +       return max(min_ticks, DIV_ROUND_CLOSEST(val, tick_duration));
>>
>> Are you sure that DIV_ROUND_CLOSEST is appropriate in those calculations
>> (instead of DIV_ROUND_UP or DIV_ROUND_DOWN) ?
>>
> [...]
> 
>>> +
>>> +       timeout_ms = total_ticks * tick_ms;
>>> +       ctrl->wdev.timeout = DIV_ROUND_CLOSEST(timeout_ms, 1000);
>>> +
>>
>> That means the actual timeout (and pretimeout) can be slightly larger
>> than the real timeout. Is this really what you want ?
> 
> Is it a problem if the WDT times out later than specified by watchdog_device.(pre)timeout?
> I can see that premature timeouts would be an issue, but I don't suppose it's problematic
> if the WDT is pinged (slightly) sooner than actually required?
> 

I am not concerned with early pings. However, if the timeout limit is set to a value
lardger than the real timeout (eg the real timeout is 25.6 seconds and the timeout
value is set to 26 seconds), the reset may occur a bit early. Granted, it doesn't
matter much, but most driver authors would ensure that the timeout is set to 25 seconds
(ie rounded down) in that situation.

> The coarsest ticks are 1342 ms, so it is not always possible to provide the requested
> (pre)timeout value, independent of the rounding scheme. Although I think it should be
> possible to replace timeout rounding by DIV_ROUND_UP (of total_ticks_ms), and pretimeout
> rounding by DIV_ROUND_DOWN (of phase2_ticks_ms), and keep stable timeouts when alternating
> between set_timeout/set_pretimeout.
> 
>>
>>> +       pretimeout_ms = phase2_ticks * tick_ms;
>>> +       ctrl->wdev.pretimeout = DIV_ROUND_CLOSEST(pretimeout_ms, 1000);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static int otto_wdt_set_timeout(struct watchdog_device *wdev, unsigned int val)
>>> +{
>>> +       struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
>>> +       unsigned long flags;
>>> +       unsigned int ret;
>>> +
>>> +       if (watchdog_timeout_invalid(wdev, val))
>>> +               return -EINVAL;
>>
>> This is not supposed to happen because the calling code already performs
>> range checks.
> 
> Right, I will drop the redundant check here and in set_pretimeout.
> 
>>
> [...]
>>> +static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long reboot_mode,
>>> +               void *data)
>>> +{
>>> +       struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
>>> +       u32 reset_mode;
>>> +       u32 v;
>>> +
>>> +       devm_free_irq(ctrl->dev, ctrl->irq_phase1, ctrl);
>>> +
>>
>> Why is this needed (instead of, say, disabling the interrupt) ?
> 
> Disabling the interrupt should actually be enough. I'll replace the devm_free_irq() with
> disable_irq(). Somehow I didn't find disable_irq(), even though that was what I was
> looking for...
> 
> [...]
>>> +
>>> +       /*
>>> +        * Since pretimeout cannot be disabled, min_timeout is twice the
>>> +        * subsystem resolution. max_timeout is 44s at a bus clock of 200MHz.
>>> +        */
>>> +       ctrl->wdev.min_timeout = 2;
>>> +       max_tick_ms = otto_wdt_tick_ms(ctrl, OTTO_WDT_PRESCALE_MAX);
>>> +       ctrl->wdev.max_timeout =
>>> +               DIV_ROUND_CLOSEST(max_tick_ms * OTTO_WDT_TIMEOUT_TICKS_MAX, 1000);
>>
>> Any reason for using max_timeout instead of max_hw_heartbeat_ms ?
> 
> I must have missed this when I was looking at watchdog_device. Makes sense to use
> max_hw_heartbeat_ms since that reflects the actual value more accurately.
> 
>>
> 
> Thanks for the feedback!
> 
> Best,
> Sander
> 

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Sander Vanheule]

On Wed, 2021-10-13 at 14:03 -0700, Guenter Roeck wrote:
> On 10/13/21 12:46 PM, Sander Vanheule wrote:
> > On Wed, 2021-10-13 at 11:48 -0700, Guenter Roeck wrote:
> > > On Wed, Oct 13, 2021 at 03:29:00PM +0200, Sander Vanheule wrote:
> > [...]
> > 
> > > > 
> > > > diff --git a/drivers/watchdog/realtek_otto_wdt.c
> > > > b/drivers/watchdog/realtek_otto_wdt.c
> > > > new file mode 100644
> > > > index 000000000000..64c9cba6b0b1
> > > > --- /dev/null
> > > > +++ b/drivers/watchdog/realtek_otto_wdt.c
> > > > @@ -0,0 +1,411 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +
> > > > +/*
> > > > + * Realtek Otto MIPS platform watchdog
> > > > + *
> > > > + * Watchdog timer that will reset the system after timeout, using the
> > > > selected
> > > > + * reset mode.
> > > > + *
> > > > + * Counter scaling and timeouts:
> > > > + * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @
> > > > 200MHz
> > > > + * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
> > > > + * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
> > > > + *   Generates an interrupt, WDT cannot be stopped after phase 1
> > > > + * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) ×
> > > > PRESCALE × T_0
> > > > + *   Resets the system according to RST_MODE
> > > 
> > > Why is there a phase2 interrupt if phase2 resets the chip ?
> > > 
> > 
> > The SoC's reset controller has an interrupt line for phase2, even though
> > then it then the
> > WDT also resets the system. I don't have any documentation about this
> > peripheral; just
> > some vendor code and there the phase2 interrupt isn't enabled. I mainly
> > added it here for
> > completeness.
> > 
> 
> It seems pointless to mandate an interrupt just for completeness.

Okay, then I will just drop it here. As I understand, the bindings should be as
complete as possible, so I think the phase2 interrupt definition should remain
there?

> 
> > One thing to note is that after CPU or software reset (not SoC reset) the
> > phase2 flag in
> > OTTO_WDT_REG_INTR will be set. That's why I always clear it in
> > otto_wdt_probe(), because
> > otherwise enabling the interrupt line would trigger otto_wdt_phase2_isr().
> > On warm
> > restarts this bit could be used to determine if there was a WDT timeout, but
> > not if the
> > WDT is configured for cold restarts (i.e. full SoC reset).
> > 
> > > 
> > [...]
> > > > +
> > > > +       raw_spin_lock_irqsave(&ctrl->lock, flags);
> > > > +       v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
> > > > +       v |= OTTO_WDT_CTRL_ENABLE;
> > > > +       iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
> > > > +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
> > > 
> > > Is it really necessary to disable interrupts for those operations ?
> > 
> > The ISR routines only use REG_INTR, which isn't modified anywhere else
> > (outside of probing
> > the device). I will replace these with raw_spin_{lock,unlock} throughout.
> > 
> 
> In that case you should not need any locks at all since the watchdog core
> ensures
> that the device is opened only once (and thus only one entity can enable or
> disable
> the watchdog).

If there is an external guarantee that at most one of {set_timeout,
set_pretimeout, enable, disable} will be called at a time, I can indeed drop the
lock. I had added the lock initially because of the read-modify-write operations
on the control register these ops perform.


> 
> > [...]
> > > > +/*
> > > > + * The timer asserts the PHASE1/PHASE2 IRQs when the number of ticks
> > > > exceeds
> > > > + * the value stored in those fields. This means the timer will run for
> > > > at least
> > > > + * one tick, so small values need to be clamped to correctly reflect
> > > > the timeout.
> > > > + */
> > > > +static inline unsigned int div_round_ticks(unsigned int val, unsigned
> > > > int
> > > > tick_duration,
> > > > +               unsigned int min_ticks)
> > > > +{
> > > > +       return max(min_ticks, DIV_ROUND_CLOSEST(val, tick_duration));
> > > 
> > > Are you sure that DIV_ROUND_CLOSEST is appropriate in those calculations
> > > (instead of DIV_ROUND_UP or DIV_ROUND_DOWN) ?
> > > 
> > [...]
> > 
> > > > +
> > > > +       timeout_ms = total_ticks * tick_ms;
> > > > +       ctrl->wdev.timeout = DIV_ROUND_CLOSEST(timeout_ms, 1000);
> > > > +
> > > 
> > > That means the actual timeout (and pretimeout) can be slightly larger
> > > than the real timeout. Is this really what you want ?
> > 
> > Is it a problem if the WDT times out later than specified by
> > watchdog_device.(pre)timeout?
> > I can see that premature timeouts would be an issue, but I don't suppose
> > it's problematic
> > if the WDT is pinged (slightly) sooner than actually required?
> > 
> 
> I am not concerned with early pings. However, if the timeout limit is set to a
> value
> lardger than the real timeout (eg the real timeout is 25.6 seconds and the
> timeout
> value is set to 26 seconds), the reset may occur a bit early. Granted, it
> doesn't
> matter much, but most driver authors would ensure that the timeout is set to
> 25 seconds
> (ie rounded down) in that situation.

I'll replace tick rounding with DIV_ROUND_UP, and timeout rounding with regular
flooring division. This results in a few timeout values being rounded up for the
coarsest tick duration, but those are then stable values.

Best,
Sander

> 
> > The coarsest ticks are 1342 ms, so it is not always possible to provide the
> > requested
> > (pre)timeout value, independent of the rounding scheme. Although I think it
> > should be
> > possible to replace timeout rounding by DIV_ROUND_UP (of total_ticks_ms),
> > and pretimeout
> > rounding by DIV_ROUND_DOWN (of phase2_ticks_ms), and keep stable timeouts
> > when alternating
> > between set_timeout/set_pretimeout.
> > 
> > > 
> > > > +       pretimeout_ms = phase2_ticks * tick_ms;
> > > > +       ctrl->wdev.pretimeout = DIV_ROUND_CLOSEST(pretimeout_ms, 1000);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int otto_wdt_set_timeout(struct watchdog_device *wdev, unsigned
> > > > int val)
> > > > +{
> > > > +       struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> > > > +       unsigned long flags;
> > > > +       unsigned int ret;
> > > > +
> > > > +       if (watchdog_timeout_invalid(wdev, val))
> > > > +               return -EINVAL;
> > > 
> > > This is not supposed to happen because the calling code already performs
> > > range checks.
> > 
> > Right, I will drop the redundant check here and in set_pretimeout.
> > 
> > > 
> > [...]
> > > > +static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long
> > > > reboot_mode,
> > > > +               void *data)
> > > > +{
> > > > +       struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
> > > > +       u32 reset_mode;
> > > > +       u32 v;
> > > > +
> > > > +       devm_free_irq(ctrl->dev, ctrl->irq_phase1, ctrl);
> > > > +
> > > 
> > > Why is this needed (instead of, say, disabling the interrupt) ?
> > 
> > Disabling the interrupt should actually be enough. I'll replace the
> > devm_free_irq() with
> > disable_irq(). Somehow I didn't find disable_irq(), even though that was
> > what I was
> > looking for...
> > 
> > [...]
> > > > +
> > > > +       /*
> > > > +        * Since pretimeout cannot be disabled, min_timeout is twice the
> > > > +        * subsystem resolution. max_timeout is 44s at a bus clock of
> > > > 200MHz.
> > > > +        */
> > > > +       ctrl->wdev.min_timeout = 2;
> > > > +       max_tick_ms = otto_wdt_tick_ms(ctrl, OTTO_WDT_PRESCALE_MAX);
> > > > +       ctrl->wdev.max_timeout =
> > > > +               DIV_ROUND_CLOSEST(max_tick_ms *
> > > > OTTO_WDT_TIMEOUT_TICKS_MAX, 1000);
> > > 
> > > Any reason for using max_timeout instead of max_hw_heartbeat_ms ?
> > 
> > I must have missed this when I was looking at watchdog_device. Makes sense
> > to use
> > max_hw_heartbeat_ms since that reflects the actual value more accurately.
> > 
> > > 
> > 
> > Thanks for the feedback!
> > 
> > Best,
> > Sander
> > 
> 

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10737 bytes --]

Hi Sander,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.15-rc5 next-20211013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Sander-Vanheule/Add-Realtek-Otto-WDT-support/20211013-213511
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f4d0cc426f77df6890aa868f96c2de89686aae8a
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/32b957f54703ffbffecc825fb8df3106b2aab6b5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Sander-Vanheule/Add-Realtek-Otto-WDT-support/20211013-213511
        git checkout 32b957f54703ffbffecc825fb8df3106b2aab6b5
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/clk/clk.c:855:6: error: redefinition of 'clk_unprepare'
     855 | void clk_unprepare(struct clk *clk)
         |      ^~~~~~~~~~~~~
   In file included from drivers/clk/clk.c:9:
   include/linux/clk.h:303:20: note: previous definition of 'clk_unprepare' with type 'void(struct clk *)'
     303 | static inline void clk_unprepare(struct clk *clk)
         |                    ^~~~~~~~~~~~~
>> drivers/clk/clk.c:936:5: error: redefinition of 'clk_prepare'
     936 | int clk_prepare(struct clk *clk)
         |     ^~~~~~~~~~~
   In file included from drivers/clk/clk.c:9:
   include/linux/clk.h:271:19: note: previous definition of 'clk_prepare' with type 'int(struct clk *)'
     271 | static inline int clk_prepare(struct clk *clk)
         |                   ^~~~~~~~~~~
>> drivers/clk/clk.c:1182:6: error: redefinition of 'clk_is_enabled_when_prepared'
    1182 | bool clk_is_enabled_when_prepared(struct clk *clk)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/clk/clk.c:9:
   include/linux/clk.h:284:20: note: previous definition of 'clk_is_enabled_when_prepared' with type 'bool(struct clk *)' {aka '_Bool(struct clk *)'}
     284 | static inline bool clk_is_enabled_when_prepared(struct clk *clk)
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for COMMON_CLK
   Depends on !HAVE_LEGACY_CLK
   Selected by
   - REALTEK_OTTO_WDT && WATCHDOG && (MACH_REALTEK_RTL || COMPILE_TEST


vim +/clk_unprepare +855 drivers/clk/clk.c

a6adc30ba7bef8 Dong Aisheng     2016-06-30  843  
4dff95dc9477a3 Stephen Boyd     2015-04-30  844  /**
4dff95dc9477a3 Stephen Boyd     2015-04-30  845   * clk_unprepare - undo preparation of a clock source
4dff95dc9477a3 Stephen Boyd     2015-04-30  846   * @clk: the clk being unprepared
4dff95dc9477a3 Stephen Boyd     2015-04-30  847   *
4dff95dc9477a3 Stephen Boyd     2015-04-30  848   * clk_unprepare may sleep, which differentiates it from clk_disable.  In a
4dff95dc9477a3 Stephen Boyd     2015-04-30  849   * simple case, clk_unprepare can be used instead of clk_disable to gate a clk
4dff95dc9477a3 Stephen Boyd     2015-04-30  850   * if the operation may sleep.  One example is a clk which is accessed over
4dff95dc9477a3 Stephen Boyd     2015-04-30  851   * I2c.  In the complex case a clk gate operation may require a fast and a slow
4dff95dc9477a3 Stephen Boyd     2015-04-30  852   * part.  It is this reason that clk_unprepare and clk_disable are not mutually
4dff95dc9477a3 Stephen Boyd     2015-04-30  853   * exclusive.  In fact clk_disable must be called before clk_unprepare.
4dff95dc9477a3 Stephen Boyd     2015-04-30  854   */
4dff95dc9477a3 Stephen Boyd     2015-04-30 @855  void clk_unprepare(struct clk *clk)
b2476490ef1113 Mike Turquette   2012-03-15  856  {
4dff95dc9477a3 Stephen Boyd     2015-04-30  857  	if (IS_ERR_OR_NULL(clk))
4dff95dc9477a3 Stephen Boyd     2015-04-30  858  		return;
b2476490ef1113 Mike Turquette   2012-03-15  859  
a6adc30ba7bef8 Dong Aisheng     2016-06-30  860  	clk_core_unprepare_lock(clk->core);
1e435256d625c2 Olof Johansson   2013-04-27  861  }
4dff95dc9477a3 Stephen Boyd     2015-04-30  862  EXPORT_SYMBOL_GPL(clk_unprepare);
1e435256d625c2 Olof Johansson   2013-04-27  863  
4dff95dc9477a3 Stephen Boyd     2015-04-30  864  static int clk_core_prepare(struct clk_core *core)
4dff95dc9477a3 Stephen Boyd     2015-04-30  865  {
4dff95dc9477a3 Stephen Boyd     2015-04-30  866  	int ret = 0;
b2476490ef1113 Mike Turquette   2012-03-15  867  
a63347251907d7 Stephen Boyd     2015-05-06  868  	lockdep_assert_held(&prepare_lock);
a63347251907d7 Stephen Boyd     2015-05-06  869  
4dff95dc9477a3 Stephen Boyd     2015-04-30  870  	if (!core)
4dff95dc9477a3 Stephen Boyd     2015-04-30  871  		return 0;
b2476490ef1113 Mike Turquette   2012-03-15  872  
4dff95dc9477a3 Stephen Boyd     2015-04-30  873  	if (core->prepare_count == 0) {
9a34b45397e5a3 Marek Szyprowski 2017-08-21  874  		ret = clk_pm_runtime_get(core);
4dff95dc9477a3 Stephen Boyd     2015-04-30  875  		if (ret)
4dff95dc9477a3 Stephen Boyd     2015-04-30  876  			return ret;
b2476490ef1113 Mike Turquette   2012-03-15  877  
9a34b45397e5a3 Marek Szyprowski 2017-08-21  878  		ret = clk_core_prepare(core->parent);
9a34b45397e5a3 Marek Szyprowski 2017-08-21  879  		if (ret)
9a34b45397e5a3 Marek Szyprowski 2017-08-21  880  			goto runtime_put;
9a34b45397e5a3 Marek Szyprowski 2017-08-21  881  
4dff95dc9477a3 Stephen Boyd     2015-04-30  882  		trace_clk_prepare(core);
1c155b3dfe0835 Ulf Hansson      2013-03-12  883  
4dff95dc9477a3 Stephen Boyd     2015-04-30  884  		if (core->ops->prepare)
4dff95dc9477a3 Stephen Boyd     2015-04-30  885  			ret = core->ops->prepare(core->hw);
1c155b3dfe0835 Ulf Hansson      2013-03-12  886  
4dff95dc9477a3 Stephen Boyd     2015-04-30  887  		trace_clk_prepare_complete(core);
b2476490ef1113 Mike Turquette   2012-03-15  888  
9a34b45397e5a3 Marek Szyprowski 2017-08-21  889  		if (ret)
9a34b45397e5a3 Marek Szyprowski 2017-08-21  890  			goto unprepare;
b2476490ef1113 Mike Turquette   2012-03-15  891  	}
b2476490ef1113 Mike Turquette   2012-03-15  892  
4dff95dc9477a3 Stephen Boyd     2015-04-30  893  	core->prepare_count++;
b2476490ef1113 Mike Turquette   2012-03-15  894  
9461f7b33d11cb Jerome Brunet    2018-06-19  895  	/*
9461f7b33d11cb Jerome Brunet    2018-06-19  896  	 * CLK_SET_RATE_GATE is a special case of clock protection
9461f7b33d11cb Jerome Brunet    2018-06-19  897  	 * Instead of a consumer claiming exclusive rate control, it is
9461f7b33d11cb Jerome Brunet    2018-06-19  898  	 * actually the provider which prevents any consumer from making any
9461f7b33d11cb Jerome Brunet    2018-06-19  899  	 * operation which could result in a rate change or rate glitch while
9461f7b33d11cb Jerome Brunet    2018-06-19  900  	 * the clock is prepared.
9461f7b33d11cb Jerome Brunet    2018-06-19  901  	 */
9461f7b33d11cb Jerome Brunet    2018-06-19  902  	if (core->flags & CLK_SET_RATE_GATE)
9461f7b33d11cb Jerome Brunet    2018-06-19  903  		clk_core_rate_protect(core);
9461f7b33d11cb Jerome Brunet    2018-06-19  904  
4dff95dc9477a3 Stephen Boyd     2015-04-30  905  	return 0;
9a34b45397e5a3 Marek Szyprowski 2017-08-21  906  unprepare:
9a34b45397e5a3 Marek Szyprowski 2017-08-21  907  	clk_core_unprepare(core->parent);
9a34b45397e5a3 Marek Szyprowski 2017-08-21  908  runtime_put:
9a34b45397e5a3 Marek Szyprowski 2017-08-21  909  	clk_pm_runtime_put(core);
9a34b45397e5a3 Marek Szyprowski 2017-08-21  910  	return ret;
b2476490ef1113 Mike Turquette   2012-03-15  911  }
b2476490ef1113 Mike Turquette   2012-03-15  912  
a6adc30ba7bef8 Dong Aisheng     2016-06-30  913  static int clk_core_prepare_lock(struct clk_core *core)
a6adc30ba7bef8 Dong Aisheng     2016-06-30  914  {
a6adc30ba7bef8 Dong Aisheng     2016-06-30  915  	int ret;
a6adc30ba7bef8 Dong Aisheng     2016-06-30  916  
a6adc30ba7bef8 Dong Aisheng     2016-06-30  917  	clk_prepare_lock();
a6adc30ba7bef8 Dong Aisheng     2016-06-30  918  	ret = clk_core_prepare(core);
a6adc30ba7bef8 Dong Aisheng     2016-06-30  919  	clk_prepare_unlock();
a6adc30ba7bef8 Dong Aisheng     2016-06-30  920  
a6adc30ba7bef8 Dong Aisheng     2016-06-30  921  	return ret;
a6adc30ba7bef8 Dong Aisheng     2016-06-30  922  }
a6adc30ba7bef8 Dong Aisheng     2016-06-30  923  
4dff95dc9477a3 Stephen Boyd     2015-04-30  924  /**
4dff95dc9477a3 Stephen Boyd     2015-04-30  925   * clk_prepare - prepare a clock source
4dff95dc9477a3 Stephen Boyd     2015-04-30  926   * @clk: the clk being prepared
4dff95dc9477a3 Stephen Boyd     2015-04-30  927   *
4dff95dc9477a3 Stephen Boyd     2015-04-30  928   * clk_prepare may sleep, which differentiates it from clk_enable.  In a simple
4dff95dc9477a3 Stephen Boyd     2015-04-30  929   * case, clk_prepare can be used instead of clk_enable to ungate a clk if the
4dff95dc9477a3 Stephen Boyd     2015-04-30  930   * operation may sleep.  One example is a clk which is accessed over I2c.  In
4dff95dc9477a3 Stephen Boyd     2015-04-30  931   * the complex case a clk ungate operation may require a fast and a slow part.
4dff95dc9477a3 Stephen Boyd     2015-04-30  932   * It is this reason that clk_prepare and clk_enable are not mutually
4dff95dc9477a3 Stephen Boyd     2015-04-30  933   * exclusive.  In fact clk_prepare must be called before clk_enable.
4dff95dc9477a3 Stephen Boyd     2015-04-30  934   * Returns 0 on success, -EERROR otherwise.
4dff95dc9477a3 Stephen Boyd     2015-04-30  935   */
4dff95dc9477a3 Stephen Boyd     2015-04-30 @936  int clk_prepare(struct clk *clk)
b2476490ef1113 Mike Turquette   2012-03-15  937  {
035a61c314eb3d Tomeu Vizoso     2015-01-23  938  	if (!clk)
4dff95dc9477a3 Stephen Boyd     2015-04-30  939  		return 0;
035a61c314eb3d Tomeu Vizoso     2015-01-23  940  
a6adc30ba7bef8 Dong Aisheng     2016-06-30  941  	return clk_core_prepare_lock(clk->core);
7ef3dcc8145263 James Hogan      2013-07-29  942  }
4dff95dc9477a3 Stephen Boyd     2015-04-30  943  EXPORT_SYMBOL_GPL(clk_prepare);
035a61c314eb3d Tomeu Vizoso     2015-01-23  944  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 58458 bytes --]

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Guenter Roeck]

On 10/14/21 3:26 AM, Sander Vanheule wrote:
> On Wed, 2021-10-13 at 14:03 -0700, Guenter Roeck wrote:
>> On 10/13/21 12:46 PM, Sander Vanheule wrote:
>>> On Wed, 2021-10-13 at 11:48 -0700, Guenter Roeck wrote:
>>>> On Wed, Oct 13, 2021 at 03:29:00PM +0200, Sander Vanheule wrote:
>>> [...]
>>>
>>>>>
>>>>> diff --git a/drivers/watchdog/realtek_otto_wdt.c
>>>>> b/drivers/watchdog/realtek_otto_wdt.c
>>>>> new file mode 100644
>>>>> index 000000000000..64c9cba6b0b1
>>>>> --- /dev/null
>>>>> +++ b/drivers/watchdog/realtek_otto_wdt.c
>>>>> @@ -0,0 +1,411 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>> +
>>>>> +/*
>>>>> + * Realtek Otto MIPS platform watchdog
>>>>> + *
>>>>> + * Watchdog timer that will reset the system after timeout, using the
>>>>> selected
>>>>> + * reset mode.
>>>>> + *
>>>>> + * Counter scaling and timeouts:
>>>>> + * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @
>>>>> 200MHz
>>>>> + * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
>>>>> + * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
>>>>> + *   Generates an interrupt, WDT cannot be stopped after phase 1
>>>>> + * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) ×
>>>>> PRESCALE × T_0
>>>>> + *   Resets the system according to RST_MODE
>>>>
>>>> Why is there a phase2 interrupt if phase2 resets the chip ?
>>>>
>>>
>>> The SoC's reset controller has an interrupt line for phase2, even though
>>> then it then the
>>> WDT also resets the system. I don't have any documentation about this
>>> peripheral; just
>>> some vendor code and there the phase2 interrupt isn't enabled. I mainly
>>> added it here for
>>> completeness.
>>>
>>
>> It seems pointless to mandate an interrupt just for completeness.
> 
> Okay, then I will just drop it here. As I understand, the bindings should be as
> complete as possible, so I think the phase2 interrupt definition should remain
> there?
> 

I still don't see the point of it if there is no known use case. At the very
least it will need to be optional, but even then I would expect a description
of the use case.

FWIW, technically I suspect that there is a means for the watchdog to generate
a second interrupt instead of resetting the hardware (otherwise the second
interrupt would not really make sense), but without hardware and without
datasheet it is impossible to confirm that.

Guenter

>>
>>> One thing to note is that after CPU or software reset (not SoC reset) the
>>> phase2 flag in
>>> OTTO_WDT_REG_INTR will be set. That's why I always clear it in
>>> otto_wdt_probe(), because
>>> otherwise enabling the interrupt line would trigger otto_wdt_phase2_isr().
>>> On warm
>>> restarts this bit could be used to determine if there was a WDT timeout, but
>>> not if the
>>> WDT is configured for cold restarts (i.e. full SoC reset).
>>>
>>>>
>>> [...]
>>>>> +
>>>>> +       raw_spin_lock_irqsave(&ctrl->lock, flags);
>>>>> +       v = ioread32(ctrl->base + OTTO_WDT_REG_CTRL);
>>>>> +       v |= OTTO_WDT_CTRL_ENABLE;
>>>>> +       iowrite32(v, ctrl->base + OTTO_WDT_REG_CTRL);
>>>>> +       raw_spin_unlock_irqrestore(&ctrl->lock, flags);
>>>>
>>>> Is it really necessary to disable interrupts for those operations ?
>>>
>>> The ISR routines only use REG_INTR, which isn't modified anywhere else
>>> (outside of probing
>>> the device). I will replace these with raw_spin_{lock,unlock} throughout.
>>>
>>
>> In that case you should not need any locks at all since the watchdog core
>> ensures
>> that the device is opened only once (and thus only one entity can enable or
>> disable
>> the watchdog).
> 
> If there is an external guarantee that at most one of {set_timeout,
> set_pretimeout, enable, disable} will be called at a time, I can indeed drop the
> lock. I had added the lock initially because of the read-modify-write operations
> on the control register these ops perform.
> 
> 
>>
>>> [...]
>>>>> +/*
>>>>> + * The timer asserts the PHASE1/PHASE2 IRQs when the number of ticks
>>>>> exceeds
>>>>> + * the value stored in those fields. This means the timer will run for
>>>>> at least
>>>>> + * one tick, so small values need to be clamped to correctly reflect
>>>>> the timeout.
>>>>> + */
>>>>> +static inline unsigned int div_round_ticks(unsigned int val, unsigned
>>>>> int
>>>>> tick_duration,
>>>>> +               unsigned int min_ticks)
>>>>> +{
>>>>> +       return max(min_ticks, DIV_ROUND_CLOSEST(val, tick_duration));
>>>>
>>>> Are you sure that DIV_ROUND_CLOSEST is appropriate in those calculations
>>>> (instead of DIV_ROUND_UP or DIV_ROUND_DOWN) ?
>>>>
>>> [...]
>>>
>>>>> +
>>>>> +       timeout_ms = total_ticks * tick_ms;
>>>>> +       ctrl->wdev.timeout = DIV_ROUND_CLOSEST(timeout_ms, 1000);
>>>>> +
>>>>
>>>> That means the actual timeout (and pretimeout) can be slightly larger
>>>> than the real timeout. Is this really what you want ?
>>>
>>> Is it a problem if the WDT times out later than specified by
>>> watchdog_device.(pre)timeout?
>>> I can see that premature timeouts would be an issue, but I don't suppose
>>> it's problematic
>>> if the WDT is pinged (slightly) sooner than actually required?
>>>
>>
>> I am not concerned with early pings. However, if the timeout limit is set to a
>> value
>> lardger than the real timeout (eg the real timeout is 25.6 seconds and the
>> timeout
>> value is set to 26 seconds), the reset may occur a bit early. Granted, it
>> doesn't
>> matter much, but most driver authors would ensure that the timeout is set to
>> 25 seconds
>> (ie rounded down) in that situation.
> 
> I'll replace tick rounding with DIV_ROUND_UP, and timeout rounding with regular
> flooring division. This results in a few timeout values being rounded up for the
> coarsest tick duration, but those are then stable values.
> 
> Best,
> Sander
> 
>>
>>> The coarsest ticks are 1342 ms, so it is not always possible to provide the
>>> requested
>>> (pre)timeout value, independent of the rounding scheme. Although I think it
>>> should be
>>> possible to replace timeout rounding by DIV_ROUND_UP (of total_ticks_ms),
>>> and pretimeout
>>> rounding by DIV_ROUND_DOWN (of phase2_ticks_ms), and keep stable timeouts
>>> when alternating
>>> between set_timeout/set_pretimeout.
>>>
>>>>
>>>>> +       pretimeout_ms = phase2_ticks * tick_ms;
>>>>> +       ctrl->wdev.pretimeout = DIV_ROUND_CLOSEST(pretimeout_ms, 1000);
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +static int otto_wdt_set_timeout(struct watchdog_device *wdev, unsigned
>>>>> int val)
>>>>> +{
>>>>> +       struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
>>>>> +       unsigned long flags;
>>>>> +       unsigned int ret;
>>>>> +
>>>>> +       if (watchdog_timeout_invalid(wdev, val))
>>>>> +               return -EINVAL;
>>>>
>>>> This is not supposed to happen because the calling code already performs
>>>> range checks.
>>>
>>> Right, I will drop the redundant check here and in set_pretimeout.
>>>
>>>>
>>> [...]
>>>>> +static int otto_wdt_restart(struct watchdog_device *wdev, unsigned long
>>>>> reboot_mode,
>>>>> +               void *data)
>>>>> +{
>>>>> +       struct otto_wdt_ctrl *ctrl = watchdog_get_drvdata(wdev);
>>>>> +       u32 reset_mode;
>>>>> +       u32 v;
>>>>> +
>>>>> +       devm_free_irq(ctrl->dev, ctrl->irq_phase1, ctrl);
>>>>> +
>>>>
>>>> Why is this needed (instead of, say, disabling the interrupt) ?
>>>
>>> Disabling the interrupt should actually be enough. I'll replace the
>>> devm_free_irq() with
>>> disable_irq(). Somehow I didn't find disable_irq(), even though that was
>>> what I was
>>> looking for...
>>>
>>> [...]
>>>>> +
>>>>> +       /*
>>>>> +        * Since pretimeout cannot be disabled, min_timeout is twice the
>>>>> +        * subsystem resolution. max_timeout is 44s at a bus clock of
>>>>> 200MHz.
>>>>> +        */
>>>>> +       ctrl->wdev.min_timeout = 2;
>>>>> +       max_tick_ms = otto_wdt_tick_ms(ctrl, OTTO_WDT_PRESCALE_MAX);
>>>>> +       ctrl->wdev.max_timeout =
>>>>> +               DIV_ROUND_CLOSEST(max_tick_ms *
>>>>> OTTO_WDT_TIMEOUT_TICKS_MAX, 1000);
>>>>
>>>> Any reason for using max_timeout instead of max_hw_heartbeat_ms ?
>>>
>>> I must have missed this when I was looking at watchdog_device. Makes sense
>>> to use
>>> max_hw_heartbeat_ms since that reflects the actual value more accurately.
>>>
>>>>
>>>
>>> Thanks for the feedback!
>>>
>>> Best,
>>> Sander
>>>
>>
> 

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Sander Vanheule]

On Thu, 2021-10-14 at 09:56 -0700, Guenter Roeck wrote:
> On 10/14/21 3:26 AM, Sander Vanheule wrote:
> > On Wed, 2021-10-13 at 14:03 -0700, Guenter Roeck wrote:
> > > On 10/13/21 12:46 PM, Sander Vanheule wrote:
> > > > On Wed, 2021-10-13 at 11:48 -0700, Guenter Roeck wrote:
> > > > > On Wed, Oct 13, 2021 at 03:29:00PM +0200, Sander Vanheule wrote:
> > > > [...]
> > > > 
> > > > > > 
> > > > > > diff --git a/drivers/watchdog/realtek_otto_wdt.c
> > > > > > b/drivers/watchdog/realtek_otto_wdt.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..64c9cba6b0b1
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/watchdog/realtek_otto_wdt.c
> > > > > > @@ -0,0 +1,411 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > +
> > > > > > +/*
> > > > > > + * Realtek Otto MIPS platform watchdog
> > > > > > + *
> > > > > > + * Watchdog timer that will reset the system after timeout, using the
> > > > > > selected
> > > > > > + * reset mode.
> > > > > > + *
> > > > > > + * Counter scaling and timeouts:
> > > > > > + * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @
> > > > > > 200MHz
> > > > > > + * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
> > > > > > + * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
> > > > > > + *   Generates an interrupt, WDT cannot be stopped after phase 1
> > > > > > + * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) ×
> > > > > > PRESCALE × T_0
> > > > > > + *   Resets the system according to RST_MODE
> > > > > 
> > > > > Why is there a phase2 interrupt if phase2 resets the chip ?
> > > > > 
> > > > 
> > > > The SoC's reset controller has an interrupt line for phase2, even though
> > > > then it then the
> > > > WDT also resets the system. I don't have any documentation about this
> > > > peripheral; just
> > > > some vendor code and there the phase2 interrupt isn't enabled. I mainly
> > > > added it here for
> > > > completeness.
> > > > 
> > > 
> > > It seems pointless to mandate an interrupt just for completeness.
> > 
> > Okay, then I will just drop it here. As I understand, the bindings should be as
> > complete as possible, so I think the phase2 interrupt definition should remain
> > there?
> > 
> 
> I still don't see the point of it if there is no known use case. At the very
> least it will need to be optional, but even then I would expect a description
> of the use case.
> 
> FWIW, technically I suspect that there is a means for the watchdog to generate
> a second interrupt instead of resetting the hardware (otherwise the second
> interrupt would not really make sense), but without hardware and without
> datasheet it is impossible to confirm that.

I haven't found any WDT reset enable/disable flag for the RTL838x and RTL839x series,
but I noticed the RTL930x series does have a potentially interesting field in their
reset controller.

WD_RST_EN: https://svanheule.net/realtek/longan/register/rst_glb_ctrl_0

Sadly we don't have any proper datasheets. All we have is the register layouts from
the source archives, which we've been trying to document ourselves. I've ordered some
hardware with an RTL9302 SoC, so I should be able to provide an update next week.

Best,
Sander

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Sander Vanheule]

On Fri, 2021-10-15 at 00:44 +0800, kernel test robot wrote:
> Hi Sander,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v5.15-rc5 next-20211013]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:   
> https://github.com/0day-ci/linux/commits/Sander-Vanheule/Add-Realtek-Otto-WDT-support/20211013-213511
> base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f4d0cc426f77df68
> 90aa868f96c2de89686aae8a
> config: sh-allmodconfig (attached as .config)
> compiler: sh4-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         #
> https://github.com/0day-ci/linux/commit/32b957f54703ffbffecc825fb8df3106b2aab6b5
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Sander-Vanheule/Add-Realtek-Otto-WDT-
> support/20211013-213511
>         git checkout 32b957f54703ffbffecc825fb8df3106b2aab6b5
>         # save the attached .config to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir
> ARCH=sh SHELL=/bin/bash drivers/
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
> > > drivers/clk/clk.c:855:6: error: redefinition of 'clk_unprepare'
>      855 | void clk_unprepare(struct clk *clk)
>          |      ^~~~~~~~~~~~~
>    In file included from drivers/clk/clk.c:9:
>    include/linux/clk.h:303:20: note: previous definition of 'clk_unprepare' with
> type 'void(struct clk *)'
>      303 | static inline void clk_unprepare(struct clk *clk)
>          |                    ^~~~~~~~~~~~~
> > > drivers/clk/clk.c:936:5: error: redefinition of 'clk_prepare'
>      936 | int clk_prepare(struct clk *clk)
>          |     ^~~~~~~~~~~
>    In file included from drivers/clk/clk.c:9:
>    include/linux/clk.h:271:19: note: previous definition of 'clk_prepare' with type
> 'int(struct clk *)'
>      271 | static inline int clk_prepare(struct clk *clk)
>          |                   ^~~~~~~~~~~
> > > drivers/clk/clk.c:1182:6: error: redefinition of 'clk_is_enabled_when_prepared'
>     1182 | bool clk_is_enabled_when_prepared(struct clk *clk)
>          |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    In file included from drivers/clk/clk.c:9:
>    include/linux/clk.h:284:20: note: previous definition of
> 'clk_is_enabled_when_prepared' with type 'bool(struct clk *)' {aka '_Bool(struct
> clk *)'}
>      284 | static inline bool clk_is_enabled_when_prepared(struct clk *clk)
>          |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Kconfig warnings: (for reference only)
>    WARNING: unmet direct dependencies detected for COMMON_CLK
>    Depends on !HAVE_LEGACY_CLK
>    Selected by
>    - REALTEK_OTTO_WDT && WATCHDOG && (MACH_REALTEK_RTL || COMPILE_TEST

I had used "select COMMON_CLK" in Kconfig, where other drivers use "depends on
COMMON_CLK". I've changed this to the latter now.

Since the tested config has both HAVE_LEGACY_CLK and COMMON_CLK set, I don't think
it's using a valid config anyway. After changing to "depends on COMMON_CLK", the
make.cross command listed above appears to build without problems.

Best,
Sander

Re: [PATCH 1/2] dt-bindings: watchdog: Realtek Otto WDT binding

[Rob Herring]

On Wed, 13 Oct 2021 15:28:59 +0200, Sander Vanheule wrote:
> Add a binding description for Realtek's watchdog timer as found on
> several of their MIPS-based SoCs (codenamed Otto), such as the RTL838x
> and RTL839x series of switch SoCs.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  .../bindings/watchdog/realtek,otto-wdt.yaml   | 89 +++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/realtek,otto-wdt.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/2] watchdog: Add Realtek Otto watchdog timer

[Sander Vanheule]

On Thu, 2021-10-14 at 09:56 -0700, Guenter Roeck wrote:
> On 10/14/21 3:26 AM, Sander Vanheule wrote:
> > On Wed, 2021-10-13 at 14:03 -0700, Guenter Roeck wrote:
> > > On 10/13/21 12:46 PM, Sander Vanheule wrote:
> > > > On Wed, 2021-10-13 at 11:48 -0700, Guenter Roeck wrote:
> > > > > On Wed, Oct 13, 2021 at 03:29:00PM +0200, Sander Vanheule wrote:
> > > > [...]
> > > > 
> > > > > > 
> > > > > > diff --git a/drivers/watchdog/realtek_otto_wdt.c
> > > > > > b/drivers/watchdog/realtek_otto_wdt.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..64c9cba6b0b1
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/watchdog/realtek_otto_wdt.c
> > > > > > @@ -0,0 +1,411 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > > > +
> > > > > > +/*
> > > > > > + * Realtek Otto MIPS platform watchdog
> > > > > > + *
> > > > > > + * Watchdog timer that will reset the system after timeout, using the
> > > > > > selected
> > > > > > + * reset mode.
> > > > > > + *
> > > > > > + * Counter scaling and timeouts:
> > > > > > + * - Base prescale of (2 << 25), providing tick duration T_0: 168ms @
> > > > > > 200MHz
> > > > > > + * - PRESCALE: logarithmic prescaler adding a factor of {1, 2, 4, 8}
> > > > > > + * - Phase 1: Times out after (PHASE1 + 1) × PRESCALE × T_0
> > > > > > + *   Generates an interrupt, WDT cannot be stopped after phase 1
> > > > > > + * - Phase 2: starts after phase 1, times out after (PHASE2 + 1) ×
> > > > > > PRESCALE × T_0
> > > > > > + *   Resets the system according to RST_MODE
> > > > > 
> > > > > Why is there a phase2 interrupt if phase2 resets the chip ?
> > > > > 
> > > > 
> > > > The SoC's reset controller has an interrupt line for phase2, even though
> > > > then it then the
> > > > WDT also resets the system. I don't have any documentation about this
> > > > peripheral; just
> > > > some vendor code and there the phase2 interrupt isn't enabled. I mainly
> > > > added it here for
> > > > completeness.
> > > > 
> > > 
> > > It seems pointless to mandate an interrupt just for completeness.
> > 
> > Okay, then I will just drop it here. As I understand, the bindings should be as
> > complete as possible, so I think the phase2 interrupt definition should remain
> > there?
> > 
> 
> I still don't see the point of it if there is no known use case. At the very
> least it will need to be optional, but even then I would expect a description
> of the use case.
> 
> FWIW, technically I suspect that there is a means for the watchdog to generate
> a second interrupt instead of resetting the hardware (otherwise the second
> interrupt would not really make sense), but without hardware and without
> datasheet it is impossible to confirm that.

After acquiring a device with an RTL9302B SoC and testing the driver, I couldn't find
any bits that disable the WDT reset and only trigger the interrupt. So I just dropped
the phase2 interrupt implementation from the v2 patches and added a note in the
commit message.

Best,
Sander