[PATCH 0/4] watchdog: add TI K3 SoC watchdog support

[PATCH 0/4] watchdog: add TI K3 SoC watchdog support

[Tero Kristo]

Hi,

This series adds support for TI K3 SoC watchdog. This watchdog operates
in windowed mode, meaning if it is petted either too early or too late
compared to its time window, it will generate an error.

Patch #2 fixes a limitation in the watchdog core this causes, as
typically the keepalive timers pet watchdogs immediately when they are
started, and with the RTI watchdog, this causes an immediate reset (too
early petting of the watchdog timer.) Alternative approach to avoid this
would be to add timer within the K3 driver itself to ignore too early
petting when the watchdog is started. Feedback welcome how people want
to see this handled.

Patch #4 should be merged via the ARM SoC tree once driver side patches
are acceptable (I can handle this portion later on.)

-Tero


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 1/4] dt-bindings: watchdog: Add support for TI K3 RTI watchdog

[Tero Kristo]

TI K3 SoCs contain an RTI (Real Time Interrupt) module which can be
used to implement a windowed watchdog functionality. Windowed watchdog
will generate an error if it is petted outside the time window, either
too early or too late.

Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 .../bindings/watchdog/ti,rti-wdt.yaml         | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
new file mode 100644
index 000000000000..3813f59fb6c3
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/ti,rti-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments K3 SoC Watchdog Timer
+
+maintainers:
+  - Tero Kristo <t-kristo@ti.com>
+
+description: |+
+  The TI K3 SoC watchdog timer is implemented via the RTI (Real Time
+  Interrupt) IP module. This timer adds a support for windowed watchdog
+  mode, which will signal an error if it is pinged outside the watchdog
+  time window, meaning either too early or too late. The error signal
+  generated can be routed to either interrupt a safety controller or
+  to directly reset the SoC.
+
+properties:
+  compatible:
+    enum:
+      - ti,rti-wdt
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+examples:
+  - |
+    /*
+     * RTI WDT in main domain on J721e SoC. Assigned clocks are used to
+     * select the source clock for the watchdog, forcing it to tick with
+     * a 32kHz clock in this case.
+     */
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    main_rti0: rti@2200000 {
+        compatible = "ti,rti-wdt";
+        reg = <0x0 0x2200000 0x0 0x100>;
+        clocks = <&k3_clks 252 1>;
+        power-domains = <&k3_pds 252 TI_SCI_PD_EXCLUSIVE>;
+        assigned-clocks = <&k3_clks 252 1>;
+        assigned-clock-parents = <&k3_clks 252 5>;
+    };
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/4] watchdog: add support for resetting keepalive timers at start

[Tero Kristo]

Current watchdog core pets the timer always after the initial keepalive
time has expired from boot-up. This is incorrect for certain timers that
don't like to be petted immediately when they are started, if they have
not been running over the boot.

To allow drivers to reset their keepalive timers during startup, add
a new watchdog flag to the api, WDOG_RESET_KEEPALIVE.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/watchdog/watchdog_dev.c | 2 ++
 include/linux/watchdog.h        | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 8b5c742f24e8..131e40c21703 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -283,6 +283,8 @@ static int watchdog_start(struct watchdog_device *wdd)
 		set_bit(WDOG_ACTIVE, &wdd->status);
 		wd_data->last_keepalive = started_at;
 		watchdog_update_worker(wdd);
+		if (test_bit(WDOG_RESET_KEEPALIVE, &wdd->status))
+			wd_data->last_hw_keepalive = started_at;
 	}
 
 	return err;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 417d9f37077a..b56e3f1b1ec3 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -116,6 +116,7 @@ struct watchdog_device {
 #define WDOG_STOP_ON_REBOOT	2	/* Should be stopped on reboot */
 #define WDOG_HW_RUNNING		3	/* True if HW watchdog running */
 #define WDOG_STOP_ON_UNREGISTER	4	/* Should be stopped on unregister */
+#define WDOG_RESET_KEEPALIVE	5	/* Reset keepalive timers at start */
 	struct list_head deferred;
 };
 
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 3/4] watchdog: Add K3 RTI watchdog support

[Tero Kristo]

Texas Instruments K3 SoCs contain an RTI (Real Time Interrupt) module
which can be used as a watchdog. This IP provides a support for
windowed watchdog mode, in which the watchdog must be petted within
a certain time window. If it is petted either too soon, or too late,
a watchdog error will be triggered.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/watchdog/Kconfig   |   8 ++
 drivers/watchdog/Makefile  |   1 +
 drivers/watchdog/rti_wdt.c | 250 +++++++++++++++++++++++++++++++++++++
 3 files changed, 259 insertions(+)
 create mode 100644 drivers/watchdog/rti_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index cec868f8db3f..81faf47d44a6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -583,6 +583,14 @@ config DAVINCI_WATCHDOG
 	  NOTE: once enabled, this timer cannot be disabled.
 	  Say N if you are unsure.
 
+config K3_RTI_WATCHDOG
+	tristate "Texas Instruments K3 RTI watchdog"
+	depends on ARCH_K3 || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  Say Y here if you want to include support for the K3 watchdog
+	  timer (RTI module) available in the K3 generation of processors.
+
 config ORION_WATCHDOG
 	tristate "Orion watchdog"
 	depends on ARCH_ORION5X || ARCH_DOVE || MACH_DOVE || ARCH_MVEBU || (COMPILE_TEST && !ARCH_EBSA110)
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2ee352bf3372..6de2e4ceef19 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_EP93XX_WATCHDOG) += ep93xx_wdt.o
 obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o
 obj-$(CONFIG_IOP_WATCHDOG) += iop_wdt.o
 obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o
+obj-$(CONFIG_K3_RTI_WATCHDOG) += rti_wdt.o
 obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o
 obj-$(CONFIG_SUNXI_WATCHDOG) += sunxi_wdt.o
 obj-$(CONFIG_RN5T618_WATCHDOG) += rn5t618_wdt.o
diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
new file mode 100644
index 000000000000..b0933b090f53
--- /dev/null
+++ b/drivers/watchdog/rti_wdt.c
@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Watchdog driver for the K3 RTI module
+ *
+ * (c) Copyright 2019 Texas Instruments Inc.
+ * All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/mod_devicetable.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/device.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/pm_runtime.h>
+
+#define MODULE_NAME "rti-wdt"
+#define DEFAULT_HEARTBEAT 60
+#define MAX_HEARTBEAT     1000
+
+/* Timer register set definition */
+#define RTIDWDCTRL	0x90
+#define RTIDWDPRLD	0x94
+#define RTIWDSTATUS	0x98
+#define RTIWDKEY	0x9c
+#define RTIDWDCNTR	0xa0
+#define RTIWWDRXCTRL	0xa4
+#define RTIWWDSIZECTRL	0xa8
+
+#define RTIWWDRX_NMI	0xa
+
+#define RTIWWDSIZE_50P	0x50
+
+#define WDENABLE_KEY	0xa98559da
+
+#define WDKEY_SEQ0		0xe51a
+#define WDKEY_SEQ1		0xa35c
+
+#define WDT_PRELOAD_SHIFT	13
+
+#define WDT_PRELOAD_MAX		0xfff
+
+#define DWDST			BIT(1)
+
+static int heartbeat;
+
+/*
+ * struct to hold data for each WDT device
+ * @base - base io address of WD device
+ * @clk - source clock of WDT
+ * @wdd - hold watchdog device as is in WDT core
+ */
+struct rti_wdt_device {
+	void __iomem		*base;
+	struct clk		*clk;
+	struct watchdog_device	wdd;
+};
+
+static int rti_wdt_start(struct watchdog_device *wdd)
+{
+	u32 timer_margin;
+	unsigned long freq;
+	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
+
+	freq = clk_get_rate(wdt->clk);
+
+	/* set timeout period */
+	timer_margin = (u64)wdd->timeout * freq;
+	timer_margin >>= WDT_PRELOAD_SHIFT;
+	if (timer_margin > WDT_PRELOAD_MAX)
+		timer_margin = WDT_PRELOAD_MAX;
+	writel_relaxed(timer_margin, wdt->base + RTIDWDPRLD);
+
+	/* Set min heartbeat to 1.1x window size */
+	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
+
+	/* Generate NMI when wdt expires */
+	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
+
+	/* Window size 50% */
+	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
+
+	readl_relaxed(wdt->base + RTIWWDSIZECTRL);
+
+	/* enable watchdog */
+	writel_relaxed(WDENABLE_KEY, wdt->base + RTIDWDCTRL);
+	return 0;
+}
+
+static int rti_wdt_ping(struct watchdog_device *wdd)
+{
+	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
+
+	/* put watchdog in service state */
+	writel_relaxed(WDKEY_SEQ0, wdt->base + RTIWDKEY);
+	/* put watchdog in active state */
+	writel_relaxed(WDKEY_SEQ1, wdt->base + RTIWDKEY);
+
+	if (readl_relaxed(wdt->base + RTIWDSTATUS))
+		WARN_ON_ONCE(1);
+
+	return 0;
+}
+
+static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	u64 timer_counter;
+	unsigned long freq;
+	u32 val;
+	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
+
+	/* if timeout has occurred then return 0 */
+	val = readl_relaxed(wdt->base + RTIWDSTATUS);
+	if (val & DWDST)
+		return 0;
+
+	freq = clk_get_rate(wdt->clk);
+	if (!freq)
+		return 0;
+
+	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
+
+	do_div(timer_counter, freq);
+
+	return timer_counter;
+}
+
+static const struct watchdog_info rti_wdt_info = {
+	.options = WDIOF_KEEPALIVEPING,
+	.identity = "K3 RTI Watchdog",
+};
+
+static const struct watchdog_ops rti_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= rti_wdt_start,
+	.ping		= rti_wdt_ping,
+	.get_timeleft	= rti_wdt_get_timeleft,
+};
+
+static int rti_wdt_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct device *dev = &pdev->dev;
+	struct resource *wdt_mem;
+	struct watchdog_device *wdd;
+	struct rti_wdt_device *wdt;
+
+	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(wdt->clk)) {
+		if (PTR_ERR(wdt->clk) != -EPROBE_DEFER)
+			dev_err(dev, "failed to get clock\n");
+		return PTR_ERR(wdt->clk);
+	}
+
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "runtime pm failed\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+
+	wdd = &wdt->wdd;
+	wdd->info = &rti_wdt_info;
+	wdd->ops = &rti_wdt_ops;
+	wdd->min_timeout = 1;
+	/* Set min heartbeat to 1.1x window size */
+	wdd->min_hw_heartbeat_ms = 11 * DEFAULT_HEARTBEAT * 1000 / 20;
+	wdd->max_hw_heartbeat_ms = MAX_HEARTBEAT * 1000;
+	wdd->timeout = DEFAULT_HEARTBEAT;
+	wdd->parent = dev;
+
+	set_bit(WDOG_RESET_KEEPALIVE, &wdd->status);
+
+	watchdog_init_timeout(wdd, heartbeat, dev);
+
+	watchdog_set_drvdata(wdd, wdt);
+	watchdog_set_nowayout(wdd, 1);
+	watchdog_set_restart_priority(wdd, 128);
+
+	wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	wdt->base = devm_ioremap_resource(dev, wdt_mem);
+	if (IS_ERR(wdt->base)) {
+		ret = PTR_ERR(wdt->base);
+		goto err_iomap;
+	}
+
+	ret = watchdog_register_device(wdd);
+	if (ret) {
+		dev_err(dev, "cannot register watchdog device\n");
+		goto err_iomap;
+	}
+
+	return 0;
+
+err_iomap:
+	pm_runtime_put_sync(&pdev->dev);
+
+	return ret;
+}
+
+static int rti_wdt_remove(struct platform_device *pdev)
+{
+	struct rti_wdt_device *wdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&wdt->wdd);
+	pm_runtime_put(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id rti_wdt_of_match[] = {
+	{ .compatible = "ti,rti-wdt", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rti_wdt_of_match);
+
+static struct platform_driver rti_wdt_driver = {
+	.driver = {
+		.name = "rti-wdt",
+		.of_match_table = rti_wdt_of_match,
+	},
+	.probe = rti_wdt_probe,
+	.remove = rti_wdt_remove,
+};
+
+module_platform_driver(rti_wdt_driver);
+
+MODULE_AUTHOR("Tero Kristo <t-kristo@ti.com>");
+MODULE_DESCRIPTION("K3 RTI Watchdog Driver");
+
+module_param(heartbeat, int, 0);
+MODULE_PARM_DESC(heartbeat,
+		 "Watchdog heartbeat period in seconds from 1 to "
+		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
+		 __MODULE_STRING(DEFAULT_HEARTBEAT));
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:rti-wdt");
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 4/4] arm64: dts: ti: k3-j721e-main: Add MAIN domain watchdog entries

[Tero Kristo]

Add DT entries for main domain watchdog0 and 1 instances.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
index 0b9d14b838a1..7ab989496c2c 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi
@@ -963,4 +963,22 @@
 
 		status = "disabled";
 	};
+
+	main_rti0: rti@2200000 {
+		compatible = "ti,rti-wdt";
+		reg = <0x0 0x2200000 0x0 0x100>;
+		clocks = <&k3_clks 252 1>;
+		power-domains = <&k3_pds 252 TI_SCI_PD_EXCLUSIVE>;
+		assigned-clocks = <&k3_clks 252 1>;
+		assigned-clock-parents = <&k3_clks 252 5>;
+	};
+
+	main_rti1: rti@2210000 {
+		compatible = "ti,rti-wdt";
+		reg = <0x0 0x2210000 0x0 0x100>;
+		clocks = <&k3_clks 253 1>;
+		power-domains = <&k3_pds 253 TI_SCI_PD_EXCLUSIVE>;
+		assigned-clocks = <&k3_clks 253 1>;
+		assigned-clock-parents = <&k3_clks 253 5>;
+	};
 };
-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/4] watchdog: add support for resetting keepalive timers at start

[Guenter Roeck]

On Fri, Feb 28, 2020 at 04:23:29PM +0200, Tero Kristo wrote:
> Current watchdog core pets the timer always after the initial keepalive
> time has expired from boot-up. This is incorrect for certain timers that
> don't like to be petted immediately when they are started, if they have
> not been running over the boot.
> 
> To allow drivers to reset their keepalive timers during startup, add
> a new watchdog flag to the api, WDOG_RESET_KEEPALIVE.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/watchdog/watchdog_dev.c | 2 ++
>  include/linux/watchdog.h        | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 8b5c742f24e8..131e40c21703 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -283,6 +283,8 @@ static int watchdog_start(struct watchdog_device *wdd)
>  		set_bit(WDOG_ACTIVE, &wdd->status);
>  		wd_data->last_keepalive = started_at;
>  		watchdog_update_worker(wdd);
> +		if (test_bit(WDOG_RESET_KEEPALIVE, &wdd->status))
> +			wd_data->last_hw_keepalive = started_at;

I don't think the additional flag is needed. The code should just set
last_hw_keepalive. After all, it already sets last_keepalive, which
determines when the next internal keepalive will be sent. It makes sense
to also set last_hw_keepalive to prevent the next keepalive from being
sent too early.

Guenter

Re: [PATCH 3/4] watchdog: Add K3 RTI watchdog support

[Guenter Roeck]

On Fri, Feb 28, 2020 at 04:23:30PM +0200, Tero Kristo wrote:
> Texas Instruments K3 SoCs contain an RTI (Real Time Interrupt) module
> which can be used as a watchdog. This IP provides a support for
> windowed watchdog mode, in which the watchdog must be petted within
> a certain time window. If it is petted either too soon, or too late,
> a watchdog error will be triggered.
> 

This needs to be explained in detail in the driver itself. It doesn't
belong into the commit description.

> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/watchdog/Kconfig   |   8 ++
>  drivers/watchdog/Makefile  |   1 +
>  drivers/watchdog/rti_wdt.c | 250 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 259 insertions(+)
>  create mode 100644 drivers/watchdog/rti_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index cec868f8db3f..81faf47d44a6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -583,6 +583,14 @@ config DAVINCI_WATCHDOG
>  	  NOTE: once enabled, this timer cannot be disabled.
>  	  Say N if you are unsure.
>  
> +config K3_RTI_WATCHDOG
> +	tristate "Texas Instruments K3 RTI watchdog"
> +	depends on ARCH_K3 || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here if you want to include support for the K3 watchdog
> +	  timer (RTI module) available in the K3 generation of processors.
> +
>  config ORION_WATCHDOG
>  	tristate "Orion watchdog"
>  	depends on ARCH_ORION5X || ARCH_DOVE || MACH_DOVE || ARCH_MVEBU || (COMPILE_TEST && !ARCH_EBSA110)
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 2ee352bf3372..6de2e4ceef19 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_EP93XX_WATCHDOG) += ep93xx_wdt.o
>  obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o
>  obj-$(CONFIG_IOP_WATCHDOG) += iop_wdt.o
>  obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o
> +obj-$(CONFIG_K3_RTI_WATCHDOG) += rti_wdt.o
>  obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o
>  obj-$(CONFIG_SUNXI_WATCHDOG) += sunxi_wdt.o
>  obj-$(CONFIG_RN5T618_WATCHDOG) += rn5t618_wdt.o
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> new file mode 100644
> index 000000000000..b0933b090f53
> --- /dev/null
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Watchdog driver for the K3 RTI module
> + *
> + * (c) Copyright 2019 Texas Instruments Inc.
> + * All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/watchdog.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/device.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/pm_runtime.h>

Alphabetic order, please.

> +
> +#define MODULE_NAME "rti-wdt"
> +#define DEFAULT_HEARTBEAT 60
> +#define MAX_HEARTBEAT     1000
> +
> +/* Timer register set definition */
> +#define RTIDWDCTRL	0x90
> +#define RTIDWDPRLD	0x94
> +#define RTIWDSTATUS	0x98
> +#define RTIWDKEY	0x9c
> +#define RTIDWDCNTR	0xa0
> +#define RTIWWDRXCTRL	0xa4
> +#define RTIWWDSIZECTRL	0xa8
> +
> +#define RTIWWDRX_NMI	0xa
> +
> +#define RTIWWDSIZE_50P	0x50
> +
> +#define WDENABLE_KEY	0xa98559da
> +
> +#define WDKEY_SEQ0		0xe51a
> +#define WDKEY_SEQ1		0xa35c
> +
> +#define WDT_PRELOAD_SHIFT	13
> +
> +#define WDT_PRELOAD_MAX		0xfff
> +
> +#define DWDST			BIT(1)
> +
> +static int heartbeat;
> +
> +/*
> + * struct to hold data for each WDT device
> + * @base - base io address of WD device
> + * @clk - source clock of WDT
> + * @wdd - hold watchdog device as is in WDT core
> + */
> +struct rti_wdt_device {
> +	void __iomem		*base;
> +	struct clk		*clk;
> +	struct watchdog_device	wdd;
> +};
> +
> +static int rti_wdt_start(struct watchdog_device *wdd)
> +{
> +	u32 timer_margin;
> +	unsigned long freq;
> +	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
> +
> +	freq = clk_get_rate(wdt->clk);
> +
Does this change at runtime ? Otherwise it would be less costly to
get the frequency in the probe function to avoid having to read it
multiple times.

> +	/* set timeout period */
> +	timer_margin = (u64)wdd->timeout * freq;
> +	timer_margin >>= WDT_PRELOAD_SHIFT;
> +	if (timer_margin > WDT_PRELOAD_MAX)
> +		timer_margin = WDT_PRELOAD_MAX;
> +	writel_relaxed(timer_margin, wdt->base + RTIDWDPRLD);
> +
So if freq is 0 (which can happen per code in rti_wdt_get_timeleft),
the value programmed into the chip will be 0.

> +	/* Set min heartbeat to 1.1x window size */
> +	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;

wdd->timeout is in seconds. If we assume it is set to 1 second,

	11 * 1 * 1000 / 20 = 550.

With 10 seconds:
	11 * 10 * 1000 / 20 = 5500

... meaning the next ping must not be earlier than 55%
of the timeout. This is highly unusual. Most watchdog daemons
will send pings earlier.

This warrants a detailed explanation. The purpose if min_hw_heartbeat_ms
is to prevent pings to be sent too closely because the HW can not handle
it otherwise. If that is what is happening here, and if pings indeed
need to be that far apart for somew reason, it needs to be documented
in the driver.

Also, "window size" is a misleading term. This is really 55%
of the configured timeout.

> +
> +	/* Generate NMI when wdt expires */
> +	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
> +

What is the expected action ? Shouldn't there be an NMI handler ?

> +	/* Window size 50% */
> +	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
> +

If the minimum ping interval is configurable, it should really be
set to a _much_ lower value. If that isn't feasible for some reason,
this reason needs to be explained in detail.

> +	readl_relaxed(wdt->base + RTIWWDSIZECTRL);
> +
> +	/* enable watchdog */
> +	writel_relaxed(WDENABLE_KEY, wdt->base + RTIDWDCTRL);
> +	return 0;
> +}
> +
> +static int rti_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
> +
> +	/* put watchdog in service state */
> +	writel_relaxed(WDKEY_SEQ0, wdt->base + RTIWDKEY);
> +	/* put watchdog in active state */
> +	writel_relaxed(WDKEY_SEQ1, wdt->base + RTIWDKEY);
> +
> +	if (readl_relaxed(wdt->base + RTIWDSTATUS))
> +		WARN_ON_ONCE(1);

What is the purpose of this warning/traceback, and what is the expected
reaction ? Some systems are set to reboot on warnings, so this may result
in a reboot. Unless such a reboot is warranted, I don't see the point of
forcing that. Have you considered dev_warn_once() instead ?

> +
> +	return 0;
> +}
> +
> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	u64 timer_counter;
> +	unsigned long freq;
> +	u32 val;
> +	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
> +
> +	/* if timeout has occurred then return 0 */
> +	val = readl_relaxed(wdt->base + RTIWDSTATUS);
> +	if (val & DWDST)
> +		return 0;
> +
> +	freq = clk_get_rate(wdt->clk);
> +	if (!freq)
> +		return 0;
> +
> +	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
> +
> +	do_div(timer_counter, freq);
> +
> +	return timer_counter;
> +}
> +
> +static const struct watchdog_info rti_wdt_info = {
> +	.options = WDIOF_KEEPALIVEPING,
> +	.identity = "K3 RTI Watchdog",
> +};
> +
> +static const struct watchdog_ops rti_wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= rti_wdt_start,
> +	.ping		= rti_wdt_ping,
> +	.get_timeleft	= rti_wdt_get_timeleft,
> +};
> +
> +static int rti_wdt_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;
> +	struct device *dev = &pdev->dev;
> +	struct resource *wdt_mem;
> +	struct watchdog_device *wdd;
> +	struct rti_wdt_device *wdt;
> +
> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	wdt->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(wdt->clk)) {
> +		if (PTR_ERR(wdt->clk) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get clock\n");
> +		return PTR_ERR(wdt->clk);
> +	}
> +
> +	pm_runtime_enable(dev);
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "runtime pm failed\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +
> +	wdd = &wdt->wdd;
> +	wdd->info = &rti_wdt_info;
> +	wdd->ops = &rti_wdt_ops;
> +	wdd->min_timeout = 1;
> +	/* Set min heartbeat to 1.1x window size */
> +	wdd->min_hw_heartbeat_ms = 11 * DEFAULT_HEARTBEAT * 1000 / 20;
> +	wdd->max_hw_heartbeat_ms = MAX_HEARTBEAT * 1000;
> +	wdd->timeout = DEFAULT_HEARTBEAT;
> +	wdd->parent = dev;
> +
> +	set_bit(WDOG_RESET_KEEPALIVE, &wdd->status);
> +
> +	watchdog_init_timeout(wdd, heartbeat, dev);
> +
> +	watchdog_set_drvdata(wdd, wdt);
> +	watchdog_set_nowayout(wdd, 1);
> +	watchdog_set_restart_priority(wdd, 128);
> +
> +	wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	wdt->base = devm_ioremap_resource(dev, wdt_mem);
> +	if (IS_ERR(wdt->base)) {
> +		ret = PTR_ERR(wdt->base);
> +		goto err_iomap;
> +	}
> +
> +	ret = watchdog_register_device(wdd);
> +	if (ret) {
> +		dev_err(dev, "cannot register watchdog device\n");
> +		goto err_iomap;
> +	}
> +
> +	return 0;
> +
> +err_iomap:
> +	pm_runtime_put_sync(&pdev->dev);
> +
> +	return ret;
> +}
> +
> +static int rti_wdt_remove(struct platform_device *pdev)
> +{
> +	struct rti_wdt_device *wdt = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&wdt->wdd);
> +	pm_runtime_put(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rti_wdt_of_match[] = {
> +	{ .compatible = "ti,rti-wdt", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rti_wdt_of_match);
> +
> +static struct platform_driver rti_wdt_driver = {
> +	.driver = {
> +		.name = "rti-wdt",
> +		.of_match_table = rti_wdt_of_match,
> +	},
> +	.probe = rti_wdt_probe,
> +	.remove = rti_wdt_remove,
> +};
> +
> +module_platform_driver(rti_wdt_driver);
> +
> +MODULE_AUTHOR("Tero Kristo <t-kristo@ti.com>");
> +MODULE_DESCRIPTION("K3 RTI Watchdog Driver");
> +
> +module_param(heartbeat, int, 0);
> +MODULE_PARM_DESC(heartbeat,
> +		 "Watchdog heartbeat period in seconds from 1 to "
> +		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
> +		 __MODULE_STRING(DEFAULT_HEARTBEAT));
> +
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:rti-wdt");
> -- 
> 2.17.1
> 
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 3/4] watchdog: Add K3 RTI watchdog support

[Tero Kristo]

On 28/02/2020 19:45, Guenter Roeck wrote:
> On Fri, Feb 28, 2020 at 04:23:30PM +0200, Tero Kristo wrote:
>> Texas Instruments K3 SoCs contain an RTI (Real Time Interrupt) module
>> which can be used as a watchdog. This IP provides a support for
>> windowed watchdog mode, in which the watchdog must be petted within
>> a certain time window. If it is petted either too soon, or too late,
>> a watchdog error will be triggered.
>>
> 
> This needs to be explained in detail in the driver itself. It doesn't
> belong into the commit description.

Ok, I can move that under the calculation logic function of the time window.

> 
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/watchdog/Kconfig   |   8 ++
>>   drivers/watchdog/Makefile  |   1 +
>>   drivers/watchdog/rti_wdt.c | 250 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 259 insertions(+)
>>   create mode 100644 drivers/watchdog/rti_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index cec868f8db3f..81faf47d44a6 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -583,6 +583,14 @@ config DAVINCI_WATCHDOG
>>   	  NOTE: once enabled, this timer cannot be disabled.
>>   	  Say N if you are unsure.
>>   
>> +config K3_RTI_WATCHDOG
>> +	tristate "Texas Instruments K3 RTI watchdog"
>> +	depends on ARCH_K3 || COMPILE_TEST
>> +	select WATCHDOG_CORE
>> +	help
>> +	  Say Y here if you want to include support for the K3 watchdog
>> +	  timer (RTI module) available in the K3 generation of processors.
>> +
>>   config ORION_WATCHDOG
>>   	tristate "Orion watchdog"
>>   	depends on ARCH_ORION5X || ARCH_DOVE || MACH_DOVE || ARCH_MVEBU || (COMPILE_TEST && !ARCH_EBSA110)
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 2ee352bf3372..6de2e4ceef19 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -57,6 +57,7 @@ obj-$(CONFIG_EP93XX_WATCHDOG) += ep93xx_wdt.o
>>   obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o
>>   obj-$(CONFIG_IOP_WATCHDOG) += iop_wdt.o
>>   obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o
>> +obj-$(CONFIG_K3_RTI_WATCHDOG) += rti_wdt.o
>>   obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o
>>   obj-$(CONFIG_SUNXI_WATCHDOG) += sunxi_wdt.o
>>   obj-$(CONFIG_RN5T618_WATCHDOG) += rn5t618_wdt.o
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> new file mode 100644
>> index 000000000000..b0933b090f53
>> --- /dev/null
>> +++ b/drivers/watchdog/rti_wdt.c
>> @@ -0,0 +1,250 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Watchdog driver for the K3 RTI module
>> + *
>> + * (c) Copyright 2019 Texas Instruments Inc.
>> + * All rights reserved.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/types.h>
>> +#include <linux/kernel.h>
>> +#include <linux/watchdog.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/device.h>
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/pm_runtime.h>
> 
> Alphabetic order, please.

Ok.

> 
>> +
>> +#define MODULE_NAME "rti-wdt"
>> +#define DEFAULT_HEARTBEAT 60
>> +#define MAX_HEARTBEAT     1000
>> +
>> +/* Timer register set definition */
>> +#define RTIDWDCTRL	0x90
>> +#define RTIDWDPRLD	0x94
>> +#define RTIWDSTATUS	0x98
>> +#define RTIWDKEY	0x9c
>> +#define RTIDWDCNTR	0xa0
>> +#define RTIWWDRXCTRL	0xa4
>> +#define RTIWWDSIZECTRL	0xa8
>> +
>> +#define RTIWWDRX_NMI	0xa
>> +
>> +#define RTIWWDSIZE_50P	0x50
>> +
>> +#define WDENABLE_KEY	0xa98559da
>> +
>> +#define WDKEY_SEQ0		0xe51a
>> +#define WDKEY_SEQ1		0xa35c
>> +
>> +#define WDT_PRELOAD_SHIFT	13
>> +
>> +#define WDT_PRELOAD_MAX		0xfff
>> +
>> +#define DWDST			BIT(1)
>> +
>> +static int heartbeat;
>> +
>> +/*
>> + * struct to hold data for each WDT device
>> + * @base - base io address of WD device
>> + * @clk - source clock of WDT
>> + * @wdd - hold watchdog device as is in WDT core
>> + */
>> +struct rti_wdt_device {
>> +	void __iomem		*base;
>> +	struct clk		*clk;
>> +	struct watchdog_device	wdd;
>> +};
>> +
>> +static int rti_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	u32 timer_margin;
>> +	unsigned long freq;
>> +	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	freq = clk_get_rate(wdt->clk);
>> +
> Does this change at runtime ? Otherwise it would be less costly to
> get the frequency in the probe function to avoid having to read it
> multiple times.

It should not change, at least I can't see why someone would want to 
change it runtime. It should be a single time configuration only via DT 
property of assigned-clocks. So yeah, I think I can safely move it to probe.

> 
>> +	/* set timeout period */
>> +	timer_margin = (u64)wdd->timeout * freq;
>> +	timer_margin >>= WDT_PRELOAD_SHIFT;
>> +	if (timer_margin > WDT_PRELOAD_MAX)
>> +		timer_margin = WDT_PRELOAD_MAX;
>> +	writel_relaxed(timer_margin, wdt->base + RTIDWDPRLD);
>> +
> So if freq is 0 (which can happen per code in rti_wdt_get_timeleft),
> the value programmed into the chip will be 0.

Freq should never be 0. I think I should return some error value in case 
it is zero in rti_wdt_get_timeleft.

> 
>> +	/* Set min heartbeat to 1.1x window size */
>> +	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
> 
> wdd->timeout is in seconds. If we assume it is set to 1 second,
> 
> 	11 * 1 * 1000 / 20 = 550.
> 
> With 10 seconds:
> 	11 * 10 * 1000 / 20 = 5500
> 
> ... meaning the next ping must not be earlier than 55%
> of the timeout. This is highly unusual. Most watchdog daemons
> will send pings earlier.

Hmm so, the comment in code is actually wrong, but the calculations are 
right and they are configured at 55% min timeout. The HW only allows 
configuring the window size down from 50% -> 25% -> 12.5%, etc. but I 
obviously selected the least strict option here which is 50%. The window 
size means the time window during which the watchdog can be serviced. 
Next step above is 100% which would result into a standard watchdog 
functionality, but is not supported by HW. >.<

> This warrants a detailed explanation. The purpose if min_hw_heartbeat_ms
> is to prevent pings to be sent too closely because the HW can not handle
> it otherwise. If that is what is happening here, and if pings indeed
> need to be that far apart for somew reason, it needs to be documented
> in the driver.

Yeah, they need to be that far apart because of HW. Let me try to 
clarify this a bit in the embedded comments for next revision.

> 
> Also, "window size" is a misleading term. This is really 55%
> of the configured timeout.

Yeah, the comment is kinda wrong.

> 
>> +
>> +	/* Generate NMI when wdt expires */
>> +	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
>> +
> 
> What is the expected action ? Shouldn't there be an NMI handler ?

The NMI in handled by the safety firmware, typical solution being system 
reboot. Generally, the behavior of the NMI here is not for Linux to 
configure.

> 
>> +	/* Window size 50% */
>> +	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
>> +
> 
> If the minimum ping interval is configurable, it should really be
> set to a _much_ lower value. If that isn't feasible for some reason,
> this reason needs to be explained in detail.

Right, but unfortunately the window can't be configured to anything 
larger as described before.

> 
>> +	readl_relaxed(wdt->base + RTIWWDSIZECTRL);
>> +
>> +	/* enable watchdog */
>> +	writel_relaxed(WDENABLE_KEY, wdt->base + RTIDWDCTRL);
>> +	return 0;
>> +}
>> +
>> +static int rti_wdt_ping(struct watchdog_device *wdd)
>> +{
>> +	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	/* put watchdog in service state */
>> +	writel_relaxed(WDKEY_SEQ0, wdt->base + RTIWDKEY);
>> +	/* put watchdog in active state */
>> +	writel_relaxed(WDKEY_SEQ1, wdt->base + RTIWDKEY);
>> +
>> +	if (readl_relaxed(wdt->base + RTIWDSTATUS))
>> +		WARN_ON_ONCE(1);
> 
> What is the purpose of this warning/traceback, and what is the expected
> reaction ? Some systems are set to reboot on warnings, so this may result
> in a reboot. Unless such a reboot is warranted, I don't see the point of
> forcing that. Have you considered dev_warn_once() instead ?

Actually, if RTIWDSTATUS is nonzero, it will typically reboot the system 
by itself, so we should never see this. Let me ditch this check as it 
doesn't really serve any purpose.

-Tero

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +	u64 timer_counter;
>> +	unsigned long freq;
>> +	u32 val;
>> +	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
>> +
>> +	/* if timeout has occurred then return 0 */
>> +	val = readl_relaxed(wdt->base + RTIWDSTATUS);
>> +	if (val & DWDST)
>> +		return 0;
>> +
>> +	freq = clk_get_rate(wdt->clk);
>> +	if (!freq)
>> +		return 0;
>> +
>> +	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
>> +
>> +	do_div(timer_counter, freq);
>> +
>> +	return timer_counter;
>> +}
>> +
>> +static const struct watchdog_info rti_wdt_info = {
>> +	.options = WDIOF_KEEPALIVEPING,
>> +	.identity = "K3 RTI Watchdog",
>> +};
>> +
>> +static const struct watchdog_ops rti_wdt_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.start		= rti_wdt_start,
>> +	.ping		= rti_wdt_ping,
>> +	.get_timeleft	= rti_wdt_get_timeleft,
>> +};
>> +
>> +static int rti_wdt_probe(struct platform_device *pdev)
>> +{
>> +	int ret = 0;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *wdt_mem;
>> +	struct watchdog_device *wdd;
>> +	struct rti_wdt_device *wdt;
>> +
>> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>> +	if (!wdt)
>> +		return -ENOMEM;
>> +
>> +	wdt->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(wdt->clk)) {
>> +		if (PTR_ERR(wdt->clk) != -EPROBE_DEFER)
>> +			dev_err(dev, "failed to get clock\n");
>> +		return PTR_ERR(wdt->clk);
>> +	}
>> +
>> +	pm_runtime_enable(dev);
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret) {
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(&pdev->dev, "runtime pm failed\n");
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, wdt);
>> +
>> +	wdd = &wdt->wdd;
>> +	wdd->info = &rti_wdt_info;
>> +	wdd->ops = &rti_wdt_ops;
>> +	wdd->min_timeout = 1;
>> +	/* Set min heartbeat to 1.1x window size */
>> +	wdd->min_hw_heartbeat_ms = 11 * DEFAULT_HEARTBEAT * 1000 / 20;
>> +	wdd->max_hw_heartbeat_ms = MAX_HEARTBEAT * 1000;
>> +	wdd->timeout = DEFAULT_HEARTBEAT;
>> +	wdd->parent = dev;
>> +
>> +	set_bit(WDOG_RESET_KEEPALIVE, &wdd->status);
>> +
>> +	watchdog_init_timeout(wdd, heartbeat, dev);
>> +
>> +	watchdog_set_drvdata(wdd, wdt);
>> +	watchdog_set_nowayout(wdd, 1);
>> +	watchdog_set_restart_priority(wdd, 128);
>> +
>> +	wdt_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	wdt->base = devm_ioremap_resource(dev, wdt_mem);
>> +	if (IS_ERR(wdt->base)) {
>> +		ret = PTR_ERR(wdt->base);
>> +		goto err_iomap;
>> +	}
>> +
>> +	ret = watchdog_register_device(wdd);
>> +	if (ret) {
>> +		dev_err(dev, "cannot register watchdog device\n");
>> +		goto err_iomap;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_iomap:
>> +	pm_runtime_put_sync(&pdev->dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rti_wdt_remove(struct platform_device *pdev)
>> +{
>> +	struct rti_wdt_device *wdt = platform_get_drvdata(pdev);
>> +
>> +	watchdog_unregister_device(&wdt->wdd);
>> +	pm_runtime_put(&pdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id rti_wdt_of_match[] = {
>> +	{ .compatible = "ti,rti-wdt", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, rti_wdt_of_match);
>> +
>> +static struct platform_driver rti_wdt_driver = {
>> +	.driver = {
>> +		.name = "rti-wdt",
>> +		.of_match_table = rti_wdt_of_match,
>> +	},
>> +	.probe = rti_wdt_probe,
>> +	.remove = rti_wdt_remove,
>> +};
>> +
>> +module_platform_driver(rti_wdt_driver);
>> +
>> +MODULE_AUTHOR("Tero Kristo <t-kristo@ti.com>");
>> +MODULE_DESCRIPTION("K3 RTI Watchdog Driver");
>> +
>> +module_param(heartbeat, int, 0);
>> +MODULE_PARM_DESC(heartbeat,
>> +		 "Watchdog heartbeat period in seconds from 1 to "
>> +		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
>> +		 __MODULE_STRING(DEFAULT_HEARTBEAT));
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:rti-wdt");
>> -- 
>> 2.17.1
>>
>> --

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/4] watchdog: add support for resetting keepalive timers at start

[Tero Kristo]

On 28/02/2020 19:13, Guenter Roeck wrote:
> On Fri, Feb 28, 2020 at 04:23:29PM +0200, Tero Kristo wrote:
>> Current watchdog core pets the timer always after the initial keepalive
>> time has expired from boot-up. This is incorrect for certain timers that
>> don't like to be petted immediately when they are started, if they have
>> not been running over the boot.
>>
>> To allow drivers to reset their keepalive timers during startup, add
>> a new watchdog flag to the api, WDOG_RESET_KEEPALIVE.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/watchdog/watchdog_dev.c | 2 ++
>>   include/linux/watchdog.h        | 1 +
>>   2 files changed, 3 insertions(+)
>>
>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index 8b5c742f24e8..131e40c21703 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -283,6 +283,8 @@ static int watchdog_start(struct watchdog_device *wdd)
>>   		set_bit(WDOG_ACTIVE, &wdd->status);
>>   		wd_data->last_keepalive = started_at;
>>   		watchdog_update_worker(wdd);
>> +		if (test_bit(WDOG_RESET_KEEPALIVE, &wdd->status))
>> +			wd_data->last_hw_keepalive = started_at;
> 
> I don't think the additional flag is needed. The code should just set
> last_hw_keepalive. After all, it already sets last_keepalive, which
> determines when the next internal keepalive will be sent. It makes sense
> to also set last_hw_keepalive to prevent the next keepalive from being
> sent too early.

Ok, I can modify this patch to tweak the last_hw_keepalive 
unconditionally if you think that is safe to do. I did it like this as 
there might be some cases where the existing implementations actually 
expect the ping to happen immediately for some reason (but I guess in 
those cases the corresponding watchdog drivers would need to be modified.)

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki