[PATCHv4 0/4] watchdog: rti-wdt: attach to running timer

[PATCHv4 0/4] watchdog: rti-wdt: attach to running timer

[Tero Kristo]

Hi,

V4 of this series only fixes 32bit compile issue with patch #3. Appears
to be compiling now fine with ARM32 allmodconfig.

-Tero


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

[PATCHv4 1/4] watchdog: use __watchdog_ping in startup

[Tero Kristo]

Current watchdog startup functionality does not respect the minimum hw
heartbeat setup and the last watchdog ping timeframe when watchdog is
already running and userspace process attaches to it. Fix this by using
the __watchdog_ping from the startup also. For this code path, we can
also let the __watchdog_ping handle the bookkeeping for the worker and
last keepalive times.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/watchdog_dev.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 7e4cd34a8c20..bc1cfa288553 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -275,15 +275,18 @@ static int watchdog_start(struct watchdog_device *wdd)
 	set_bit(_WDOG_KEEPALIVE, &wd_data->status);
 
 	started_at = ktime_get();
-	if (watchdog_hw_running(wdd) && wdd->ops->ping)
-		err = wdd->ops->ping(wdd);
-	else
+	if (watchdog_hw_running(wdd) && wdd->ops->ping) {
+		err = __watchdog_ping(wdd);
+		if (err == 0)
+			set_bit(WDOG_ACTIVE, &wdd->status);
+	} else {
 		err = wdd->ops->start(wdd);
-	if (err == 0) {
-		set_bit(WDOG_ACTIVE, &wdd->status);
-		wd_data->last_keepalive = started_at;
-		wd_data->last_hw_keepalive = started_at;
-		watchdog_update_worker(wdd);
+		if (err == 0) {
+			set_bit(WDOG_ACTIVE, &wdd->status);
+			wd_data->last_keepalive = started_at;
+			wd_data->last_hw_keepalive = started_at;
+			watchdog_update_worker(wdd);
+		}
 	}
 
 	return err;
-- 
2.17.1

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

[PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time

[Tero Kristo]

Certain watchdogs require the watchdog only to be pinged within a
specific time window, pinging too early or too late cause the watchdog
to fire. In cases where this sort of watchdog has been started before
kernel comes up, we must adjust the watchdog keepalive window to match
the actually running timer, so add a new driver API for this purpose.

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

diff --git a/Documentation/watchdog/watchdog-kernel-api.rst b/Documentation/watchdog/watchdog-kernel-api.rst
index 068a55ee0d4a..baf44e986b07 100644
--- a/Documentation/watchdog/watchdog-kernel-api.rst
+++ b/Documentation/watchdog/watchdog-kernel-api.rst
@@ -336,3 +336,15 @@ an action is taken by a preconfigured pretimeout governor preassigned to
 the watchdog device. If watchdog pretimeout governor framework is not
 enabled, watchdog_notify_pretimeout() prints a notification message to
 the kernel log buffer.
+
+To set the last known HW keepalive time for a watchdog, the following function
+should be used::
+
+  int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
+                                     unsigned int last_ping_ms)
+
+This function must be called immediately after watchdog registration. It
+sets the last known hardware heartbeat to have happened last_ping_ms before
+current time. Calling this is only needed if the watchdog is already running
+when probe is called, and the watchdog can only be pinged after the
+min_hw_heartbeat_ms time has passed from the last ping.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index bc1cfa288553..e74a0c6811b5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -1138,6 +1138,36 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)
 	watchdog_cdev_unregister(wdd);
 }
 
+/*
+ *	watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog
+ *	@wdd: watchdog device
+ *	@last_ping_ms: time since last HW heartbeat
+ *
+ *	Adjusts the last known HW keepalive time for a watchdog timer.
+ *	This is needed if the watchdog is already running when the probe
+ *	function is called, and it can't be pinged immediately. This
+ *	function must be called immediately after watchdog registration,
+ *	and min_hw_heartbeat_ms must be set for this to be useful.
+ */
+int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
+				   unsigned int last_ping_ms)
+{
+	struct watchdog_core_data *wd_data;
+	ktime_t now;
+
+	if (!wdd)
+		return -EINVAL;
+
+	wd_data = wdd->wd_data;
+
+	now = ktime_get();
+
+	wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms));
+
+	return __watchdog_ping(wdd);
+}
+EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive);
+
 /*
  *	watchdog_dev_init: init dev part of watchdog core
  *
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 1464ce6ffa31..9b19e6bb68b5 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -210,6 +210,8 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
 extern int watchdog_register_device(struct watchdog_device *);
 extern void watchdog_unregister_device(struct watchdog_device *);
 
+int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int);
+
 /* devres register variant */
 int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);
 
-- 
2.17.1

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

[PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe

[Tero Kristo]

If the RTI watchdog is running already during probe, the driver must
configure itself to match the HW. Window size and timeout is probed from
hardware, and the last keepalive ping is adjusted to match it also.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++----
 1 file changed, 102 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index d456dd72d99a..7cbdc178ffe8 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -35,7 +35,11 @@
 
 #define RTIWWDRX_NMI	0xa
 
-#define RTIWWDSIZE_50P	0x50
+#define RTIWWDSIZE_50P		0x50
+#define RTIWWDSIZE_25P		0x500
+#define RTIWWDSIZE_12P5		0x5000
+#define RTIWWDSIZE_6P25		0x50000
+#define RTIWWDSIZE_3P125	0x500000
 
 #define WDENABLE_KEY	0xa98559da
 
@@ -48,7 +52,7 @@
 
 #define DWDST			BIT(1)
 
-static int heartbeat;
+static int heartbeat = DEFAULT_HEARTBEAT;
 
 /*
  * struct to hold data for each WDT device
@@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
 	 * be petted during the open window; not too early or not too late.
 	 * The HW configuration options only allow for the open window size
 	 * to be 50% or less than that; we obviouly want to configure the open
-	 * window as large as possible so we select the 50% option. To avoid
-	 * any glitches, we accommodate 5% safety margin also, so we setup
-	 * the min_hw_hearbeat at 55% of the timeout period.
+	 * window as large as possible so we select the 50% option.
 	 */
-	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
+	wdd->min_hw_heartbeat_ms = 500 * wdd->timeout;
 
 	/* Generate NMI when wdt expires */
 	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
@@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd)
 	return 0;
 }
 
-static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
+static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize)
+{
+	/*
+	 * RTI only supports a windowed mode, where the watchdog can only
+	 * be petted during the open window; not too early or not too late.
+	 * The HW configuration options only allow for the open window size
+	 * to be 50% or less than that.
+	 */
+	switch (wsize) {
+	case RTIWWDSIZE_50P:
+		/* 50% open window => 50% min heartbeat */
+		wdd->min_hw_heartbeat_ms = 500 * heartbeat;
+		break;
+
+	case RTIWWDSIZE_25P:
+		/* 25% open window => 75% min heartbeat */
+		wdd->min_hw_heartbeat_ms = 750 * heartbeat;
+		break;
+
+	case RTIWWDSIZE_12P5:
+		/* 12.5% open window => 87.5% min heartbeat */
+		wdd->min_hw_heartbeat_ms = 875 * heartbeat;
+		break;
+
+	case RTIWWDSIZE_6P25:
+		/* 6.5% open window => 93.5% min heartbeat */
+		wdd->min_hw_heartbeat_ms = 935 * heartbeat;
+		break;
+
+	case RTIWWDSIZE_3P125:
+		/* 3.125% open window => 96.9% min heartbeat */
+		wdd->min_hw_heartbeat_ms = 969 * heartbeat;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd)
 {
 	u64 timer_counter;
 	u32 val;
@@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
 
 	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
 
+	timer_counter *= 1000;
+
 	do_div(timer_counter, wdt->freq);
 
 	return timer_counter;
 }
 
+static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	return rti_wdt_get_timeleft_ms(wdd) / 1000;
+}
+
 static const struct watchdog_info rti_wdt_info = {
 	.options = WDIOF_KEEPALIVEPING,
 	.identity = "K3 RTI Watchdog",
@@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	struct watchdog_device *wdd;
 	struct rti_wdt_device *wdt;
 	struct clk *clk;
+	u32 last_ping = 0;
 
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
@@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	/*
+	 * If watchdog is running at 32k clock, it is not accurate.
+	 * Adjust frequency down in this case so that we don't pet
+	 * the watchdog too often.
+	 */
+	if (wdt->freq < 32768)
+		wdt->freq = wdt->freq * 9 / 10;
+
 	pm_runtime_enable(dev);
 	ret = pm_runtime_get_sync(dev);
 	if (ret) {
@@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	wdd->min_timeout = 1;
 	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
 		wdt->freq * 1000;
-	wdd->timeout = DEFAULT_HEARTBEAT;
 	wdd->parent = dev;
 
-	watchdog_init_timeout(wdd, heartbeat, dev);
-
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, 1);
 	watchdog_set_restart_priority(wdd, 128);
@@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		goto err_iomap;
 	}
 
+	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
+		u32 time_left_ms;
+		u64 heartbeat_ms;
+		u32 wsize;
+
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+		time_left_ms = rti_wdt_get_timeleft_ms(wdd);
+		heartbeat_ms = readl(wdt->base + RTIDWDPRLD);
+		heartbeat_ms <<= WDT_PRELOAD_SHIFT;
+		heartbeat_ms *= 1000;
+		do_div(heartbeat_ms, wdt->freq);
+		if (heartbeat_ms != heartbeat * 1000)
+			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
+
+		heartbeat = heartbeat_ms;
+		heartbeat /= 1000;
+
+		wsize = readl(wdt->base + RTIWWDSIZECTRL);
+		ret = rti_wdt_setup_hw_hb(wdd, wsize);
+		if (ret) {
+			dev_err(dev, "bad window size.\n");
+			goto err_iomap;
+		}
+
+		last_ping = heartbeat_ms - time_left_ms;
+		if (time_left_ms > heartbeat_ms) {
+			dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n");
+			last_ping = 0;
+		}
+	}
+
+	watchdog_init_timeout(wdd, heartbeat, dev);
+
 	ret = watchdog_register_device(wdd);
 	if (ret) {
 		dev_err(dev, "cannot register watchdog device\n");
 		goto err_iomap;
 	}
 
+	if (last_ping)
+		watchdog_set_last_hw_keepalive(wdd, last_ping);
+
 	return 0;
 
 err_iomap:
-- 
2.17.1

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

[PATCHv4 4/4] watchdog: rti-wdt: balance pm runtime enable calls

[Tero Kristo]

PM runtime should be disabled in the fail path of probe and when
the driver is removed.

Fixes: 2d63908bdbfb ("watchdog: Add K3 RTI watchdog support")
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/rti_wdt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index 7cbdc178ffe8..705e8f7523e8 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -303,6 +303,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
 
 err_iomap:
 	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 
 	return ret;
 }
@@ -313,6 +314,7 @@ static int rti_wdt_remove(struct platform_device *pdev)
 
 	watchdog_unregister_device(&wdt->wdd);
 	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
-- 
2.17.1

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

Re: [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time

[Guenter Roeck]

On 7/17/20 6:29 AM, Tero Kristo wrote:
> Certain watchdogs require the watchdog only to be pinged within a
> specific time window, pinging too early or too late cause the watchdog
> to fire. In cases where this sort of watchdog has been started before
> kernel comes up, we must adjust the watchdog keepalive window to match
> the actually running timer, so add a new driver API for this purpose.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  .../watchdog/watchdog-kernel-api.rst          | 12 ++++++++
>  drivers/watchdog/watchdog_dev.c               | 30 +++++++++++++++++++
>  include/linux/watchdog.h                      |  2 ++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/Documentation/watchdog/watchdog-kernel-api.rst b/Documentation/watchdog/watchdog-kernel-api.rst
> index 068a55ee0d4a..baf44e986b07 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.rst
> +++ b/Documentation/watchdog/watchdog-kernel-api.rst
> @@ -336,3 +336,15 @@ an action is taken by a preconfigured pretimeout governor preassigned to
>  the watchdog device. If watchdog pretimeout governor framework is not
>  enabled, watchdog_notify_pretimeout() prints a notification message to
>  the kernel log buffer.
> +
> +To set the last known HW keepalive time for a watchdog, the following function
> +should be used::
> +
> +  int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
> +                                     unsigned int last_ping_ms)
> +
> +This function must be called immediately after watchdog registration. It
> +sets the last known hardware heartbeat to have happened last_ping_ms before
> +current time. Calling this is only needed if the watchdog is already running
> +when probe is called, and the watchdog can only be pinged after the
> +min_hw_heartbeat_ms time has passed from the last ping.
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index bc1cfa288553..e74a0c6811b5 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -1138,6 +1138,36 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)
>  	watchdog_cdev_unregister(wdd);
>  }
>  
> +/*
> + *	watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog
> + *	@wdd: watchdog device
> + *	@last_ping_ms: time since last HW heartbeat
> + *
> + *	Adjusts the last known HW keepalive time for a watchdog timer.
> + *	This is needed if the watchdog is already running when the probe
> + *	function is called, and it can't be pinged immediately. This
> + *	function must be called immediately after watchdog registration,
> + *	and min_hw_heartbeat_ms must be set for this to be useful.
> + */
> +int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
> +				   unsigned int last_ping_ms)
> +{
> +	struct watchdog_core_data *wd_data;
> +	ktime_t now;
> +
> +	if (!wdd)
> +		return -EINVAL;
> +
> +	wd_data = wdd->wd_data;
> +
> +	now = ktime_get();
> +
> +	wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms));
> +
> +	return __watchdog_ping(wdd);
> +}
> +EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive);
> +
>  /*
>   *	watchdog_dev_init: init dev part of watchdog core
>   *
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 1464ce6ffa31..9b19e6bb68b5 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -210,6 +210,8 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
>  extern int watchdog_register_device(struct watchdog_device *);
>  extern void watchdog_unregister_device(struct watchdog_device *);
>  
> +int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int);
> +
>  /* devres register variant */
>  int devm_watchdog_register_device(struct device *dev, struct watchdog_device *);
>  
> 

Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe

[Guenter Roeck]

On 7/17/20 6:29 AM, Tero Kristo wrote:
> If the RTI watchdog is running already during probe, the driver must
> configure itself to match the HW. Window size and timeout is probed from
> hardware, and the last keepalive ping is adjusted to match it also.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++----
>  1 file changed, 102 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index d456dd72d99a..7cbdc178ffe8 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -35,7 +35,11 @@
>  
>  #define RTIWWDRX_NMI	0xa
>  
> -#define RTIWWDSIZE_50P	0x50
> +#define RTIWWDSIZE_50P		0x50
> +#define RTIWWDSIZE_25P		0x500
> +#define RTIWWDSIZE_12P5		0x5000
> +#define RTIWWDSIZE_6P25		0x50000
> +#define RTIWWDSIZE_3P125	0x500000
>  
>  #define WDENABLE_KEY	0xa98559da
>  
> @@ -48,7 +52,7 @@
>  
>  #define DWDST			BIT(1)
>  
> -static int heartbeat;
> +static int heartbeat = DEFAULT_HEARTBEAT;
>  
>  /*
>   * struct to hold data for each WDT device
> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
>  	 * be petted during the open window; not too early or not too late.
>  	 * The HW configuration options only allow for the open window size
>  	 * to be 50% or less than that; we obviouly want to configure the open
> -	 * window as large as possible so we select the 50% option. To avoid
> -	 * any glitches, we accommodate 5% safety margin also, so we setup
> -	 * the min_hw_hearbeat at 55% of the timeout period.
> +	 * window as large as possible so we select the 50% option.
>  	 */
> -	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
> +	wdd->min_hw_heartbeat_ms = 500 * wdd->timeout;
>  
>  	/* Generate NMI when wdt expires */
>  	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd)
>  	return 0;
>  }
>  
> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize)
> +{
> +	/*
> +	 * RTI only supports a windowed mode, where the watchdog can only
> +	 * be petted during the open window; not too early or not too late.
> +	 * The HW configuration options only allow for the open window size
> +	 * to be 50% or less than that.
> +	 */
> +	switch (wsize) {
> +	case RTIWWDSIZE_50P:
> +		/* 50% open window => 50% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 500 * heartbeat;
> +		break;
> +
> +	case RTIWWDSIZE_25P:
> +		/* 25% open window => 75% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 750 * heartbeat;
> +		break;
> +
> +	case RTIWWDSIZE_12P5:
> +		/* 12.5% open window => 87.5% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 875 * heartbeat;
> +		break;
> +
> +	case RTIWWDSIZE_6P25:
> +		/* 6.5% open window => 93.5% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 935 * heartbeat;
> +		break;
> +
> +	case RTIWWDSIZE_3P125:
> +		/* 3.125% open window => 96.9% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 969 * heartbeat;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd)
>  {
>  	u64 timer_counter;
>  	u32 val;
> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>  
>  	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
>  
> +	timer_counter *= 1000;
> +
>  	do_div(timer_counter, wdt->freq);
>  
>  	return timer_counter;
>  }
>  
> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	return rti_wdt_get_timeleft_ms(wdd) / 1000;
> +}
> +
>  static const struct watchdog_info rti_wdt_info = {
>  	.options = WDIOF_KEEPALIVEPING,
>  	.identity = "K3 RTI Watchdog",
> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  	struct watchdog_device *wdd;
>  	struct rti_wdt_device *wdt;
>  	struct clk *clk;
> +	u32 last_ping = 0;
>  
>  	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>  	if (!wdt)
> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * If watchdog is running at 32k clock, it is not accurate.
> +	 * Adjust frequency down in this case so that we don't pet
> +	 * the watchdog too often.
> +	 */
> +	if (wdt->freq < 32768)
> +		wdt->freq = wdt->freq * 9 / 10;
> +
>  	pm_runtime_enable(dev);
>  	ret = pm_runtime_get_sync(dev);
>  	if (ret) {
> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  	wdd->min_timeout = 1;
>  	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>  		wdt->freq * 1000;
> -	wdd->timeout = DEFAULT_HEARTBEAT;
>  	wdd->parent = dev;
>  
> -	watchdog_init_timeout(wdd, heartbeat, dev);
> -
>  	watchdog_set_drvdata(wdd, wdt);
>  	watchdog_set_nowayout(wdd, 1);
>  	watchdog_set_restart_priority(wdd, 128);
> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  		goto err_iomap;
>  	}
>  
> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
> +		u32 time_left_ms;
> +		u64 heartbeat_ms;
> +		u32 wsize;
> +
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +		time_left_ms = rti_wdt_get_timeleft_ms(wdd);
> +		heartbeat_ms = readl(wdt->base + RTIDWDPRLD);
> +		heartbeat_ms <<= WDT_PRELOAD_SHIFT;
> +		heartbeat_ms *= 1000;
> +		do_div(heartbeat_ms, wdt->freq);
> +		if (heartbeat_ms != heartbeat * 1000)
> +			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
> +
> +		heartbeat = heartbeat_ms;
> +		heartbeat /= 1000;
> +
> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);
> +		ret = rti_wdt_setup_hw_hb(wdd, wsize);
> +		if (ret) {
> +			dev_err(dev, "bad window size.\n");
> +			goto err_iomap;
> +		}
> +
> +		last_ping = heartbeat_ms - time_left_ms;
> +		if (time_left_ms > heartbeat_ms) {
> +			dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n");
> +			last_ping = 0;
> +		}
> +	}
> +
> +	watchdog_init_timeout(wdd, heartbeat, dev);
> +
>  	ret = watchdog_register_device(wdd);
>  	if (ret) {
>  		dev_err(dev, "cannot register watchdog device\n");
>  		goto err_iomap;
>  	}
>  
> +	if (last_ping)
> +		watchdog_set_last_hw_keepalive(wdd, last_ping);
> +
>  	return 0;
>  
>  err_iomap:
> 

Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe

[Jan Kiszka]

On 17.07.20 15:29, Tero Kristo wrote:
> If the RTI watchdog is running already during probe, the driver must
> configure itself to match the HW. Window size and timeout is probed from
> hardware, and the last keepalive ping is adjusted to match it also.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++----
>  1 file changed, 102 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index d456dd72d99a..7cbdc178ffe8 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -35,7 +35,11 @@
>  
>  #define RTIWWDRX_NMI	0xa
>  
> -#define RTIWWDSIZE_50P	0x50
> +#define RTIWWDSIZE_50P		0x50
> +#define RTIWWDSIZE_25P		0x500
> +#define RTIWWDSIZE_12P5		0x5000
> +#define RTIWWDSIZE_6P25		0x50000
> +#define RTIWWDSIZE_3P125	0x500000
>  
>  #define WDENABLE_KEY	0xa98559da
>  
> @@ -48,7 +52,7 @@
>  
>  #define DWDST			BIT(1)
>  
> -static int heartbeat;
> +static int heartbeat = DEFAULT_HEARTBEAT;
>  
>  /*
>   * struct to hold data for each WDT device
> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
>  	 * be petted during the open window; not too early or not too late.
>  	 * The HW configuration options only allow for the open window size
>  	 * to be 50% or less than that; we obviouly want to configure the open
> -	 * window as large as possible so we select the 50% option. To avoid
> -	 * any glitches, we accommodate 5% safety margin also, so we setup
> -	 * the min_hw_hearbeat at 55% of the timeout period.
> +	 * window as large as possible so we select the 50% option.
>  	 */
> -	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
> +	wdd->min_hw_heartbeat_ms = 500 * wdd->timeout;
>  
>  	/* Generate NMI when wdt expires */
>  	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd)
>  	return 0;
>  }
>  
> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize)
> +{
> +	/*
> +	 * RTI only supports a windowed mode, where the watchdog can only
> +	 * be petted during the open window; not too early or not too late.
> +	 * The HW configuration options only allow for the open window size
> +	 * to be 50% or less than that.
> +	 */
> +	switch (wsize) {
> +	case RTIWWDSIZE_50P:
> +		/* 50% open window => 50% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 500 * heartbeat;
> +		break;
> +
> +	case RTIWWDSIZE_25P:
> +		/* 25% open window => 75% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 750 * heartbeat;
> +		break;
> +
> +	case RTIWWDSIZE_12P5:
> +		/* 12.5% open window => 87.5% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 875 * heartbeat;
> +		break;
> +
> +	case RTIWWDSIZE_6P25:
> +		/* 6.5% open window => 93.5% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 935 * heartbeat;
> +		break;
> +
> +	case RTIWWDSIZE_3P125:
> +		/* 3.125% open window => 96.9% min heartbeat */
> +		wdd->min_hw_heartbeat_ms = 969 * heartbeat;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd)
>  {
>  	u64 timer_counter;
>  	u32 val;
> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>  
>  	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
>  
> +	timer_counter *= 1000;
> +
>  	do_div(timer_counter, wdt->freq);
>  
>  	return timer_counter;
>  }
>  
> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	return rti_wdt_get_timeleft_ms(wdd) / 1000;
> +}
> +
>  static const struct watchdog_info rti_wdt_info = {
>  	.options = WDIOF_KEEPALIVEPING,
>  	.identity = "K3 RTI Watchdog",
> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  	struct watchdog_device *wdd;
>  	struct rti_wdt_device *wdt;
>  	struct clk *clk;
> +	u32 last_ping = 0;
>  
>  	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>  	if (!wdt)
> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * If watchdog is running at 32k clock, it is not accurate.
> +	 * Adjust frequency down in this case so that we don't pet
> +	 * the watchdog too often.
> +	 */
> +	if (wdt->freq < 32768)
> +		wdt->freq = wdt->freq * 9 / 10;
> +

This seems broken: You are only adjusting the frequency value used by
the driver. What has been programmed into the hardware already is still
based on real frequency. Moreover, this path is not only taken when the
watchdog is already running - but the latter case is what the subject
and commit message suggests. I've noticed this by comparing
bootloader-programmed values with those the driver assumes to see - 10%
off, obviously.

So, what is actually supposed to happen here?

Jan

>  	pm_runtime_enable(dev);
>  	ret = pm_runtime_get_sync(dev);
>  	if (ret) {
> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  	wdd->min_timeout = 1;
>  	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>  		wdt->freq * 1000;
> -	wdd->timeout = DEFAULT_HEARTBEAT;
>  	wdd->parent = dev;
>  
> -	watchdog_init_timeout(wdd, heartbeat, dev);
> -
>  	watchdog_set_drvdata(wdd, wdt);
>  	watchdog_set_nowayout(wdd, 1);
>  	watchdog_set_restart_priority(wdd, 128);
> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  		goto err_iomap;
>  	}
>  
> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
> +		u32 time_left_ms;
> +		u64 heartbeat_ms;
> +		u32 wsize;
> +
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +		time_left_ms = rti_wdt_get_timeleft_ms(wdd);
> +		heartbeat_ms = readl(wdt->base + RTIDWDPRLD);
> +		heartbeat_ms <<= WDT_PRELOAD_SHIFT;
> +		heartbeat_ms *= 1000;
> +		do_div(heartbeat_ms, wdt->freq);
> +		if (heartbeat_ms != heartbeat * 1000)
> +			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
> +
> +		heartbeat = heartbeat_ms;
> +		heartbeat /= 1000;
> +
> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);
> +		ret = rti_wdt_setup_hw_hb(wdd, wsize);
> +		if (ret) {
> +			dev_err(dev, "bad window size.\n");
> +			goto err_iomap;
> +		}
> +
> +		last_ping = heartbeat_ms - time_left_ms;
> +		if (time_left_ms > heartbeat_ms) {
> +			dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n");
> +			last_ping = 0;
> +		}
> +	}
> +
> +	watchdog_init_timeout(wdd, heartbeat, dev);
> +
>  	ret = watchdog_register_device(wdd);
>  	if (ret) {
>  		dev_err(dev, "cannot register watchdog device\n");
>  		goto err_iomap;
>  	}
>  
> +	if (last_ping)
> +		watchdog_set_last_hw_keepalive(wdd, last_ping);
> +
>  	return 0;
>  
>  err_iomap:

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe

[Jan Kiszka]

On 21.02.22 10:10, Jan Kiszka wrote:
> On 17.07.20 15:29, Tero Kristo wrote:
>> If the RTI watchdog is running already during probe, the driver must
>> configure itself to match the HW. Window size and timeout is probed from
>> hardware, and the last keepalive ping is adjusted to match it also.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>  drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++----
>>  1 file changed, 102 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> index d456dd72d99a..7cbdc178ffe8 100644
>> --- a/drivers/watchdog/rti_wdt.c
>> +++ b/drivers/watchdog/rti_wdt.c
>> @@ -35,7 +35,11 @@
>>  
>>  #define RTIWWDRX_NMI	0xa
>>  
>> -#define RTIWWDSIZE_50P	0x50
>> +#define RTIWWDSIZE_50P		0x50
>> +#define RTIWWDSIZE_25P		0x500
>> +#define RTIWWDSIZE_12P5		0x5000
>> +#define RTIWWDSIZE_6P25		0x50000
>> +#define RTIWWDSIZE_3P125	0x500000
>>  
>>  #define WDENABLE_KEY	0xa98559da
>>  
>> @@ -48,7 +52,7 @@
>>  
>>  #define DWDST			BIT(1)
>>  
>> -static int heartbeat;
>> +static int heartbeat = DEFAULT_HEARTBEAT;
>>  
>>  /*
>>   * struct to hold data for each WDT device
>> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
>>  	 * be petted during the open window; not too early or not too late.
>>  	 * The HW configuration options only allow for the open window size
>>  	 * to be 50% or less than that; we obviouly want to configure the open
>> -	 * window as large as possible so we select the 50% option. To avoid
>> -	 * any glitches, we accommodate 5% safety margin also, so we setup
>> -	 * the min_hw_hearbeat at 55% of the timeout period.
>> +	 * window as large as possible so we select the 50% option.
>>  	 */
>> -	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
>> +	wdd->min_hw_heartbeat_ms = 500 * wdd->timeout;
>>  
>>  	/* Generate NMI when wdt expires */
>>  	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
>> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd)
>>  	return 0;
>>  }
>>  
>> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize)
>> +{
>> +	/*
>> +	 * RTI only supports a windowed mode, where the watchdog can only
>> +	 * be petted during the open window; not too early or not too late.
>> +	 * The HW configuration options only allow for the open window size
>> +	 * to be 50% or less than that.
>> +	 */
>> +	switch (wsize) {
>> +	case RTIWWDSIZE_50P:
>> +		/* 50% open window => 50% min heartbeat */
>> +		wdd->min_hw_heartbeat_ms = 500 * heartbeat;
>> +		break;
>> +
>> +	case RTIWWDSIZE_25P:
>> +		/* 25% open window => 75% min heartbeat */
>> +		wdd->min_hw_heartbeat_ms = 750 * heartbeat;
>> +		break;
>> +
>> +	case RTIWWDSIZE_12P5:
>> +		/* 12.5% open window => 87.5% min heartbeat */
>> +		wdd->min_hw_heartbeat_ms = 875 * heartbeat;
>> +		break;
>> +
>> +	case RTIWWDSIZE_6P25:
>> +		/* 6.5% open window => 93.5% min heartbeat */
>> +		wdd->min_hw_heartbeat_ms = 935 * heartbeat;
>> +		break;
>> +
>> +	case RTIWWDSIZE_3P125:
>> +		/* 3.125% open window => 96.9% min heartbeat */
>> +		wdd->min_hw_heartbeat_ms = 969 * heartbeat;
>> +		break;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd)
>>  {
>>  	u64 timer_counter;
>>  	u32 val;
>> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>>  
>>  	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
>>  
>> +	timer_counter *= 1000;
>> +
>>  	do_div(timer_counter, wdt->freq);
>>  
>>  	return timer_counter;
>>  }
>>  
>> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> +	return rti_wdt_get_timeleft_ms(wdd) / 1000;
>> +}
>> +
>>  static const struct watchdog_info rti_wdt_info = {
>>  	.options = WDIOF_KEEPALIVEPING,
>>  	.identity = "K3 RTI Watchdog",
>> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>  	struct watchdog_device *wdd;
>>  	struct rti_wdt_device *wdt;
>>  	struct clk *clk;
>> +	u32 last_ping = 0;
>>  
>>  	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>  	if (!wdt)
>> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>  		return -EINVAL;
>>  	}
>>  
>> +	/*
>> +	 * If watchdog is running at 32k clock, it is not accurate.
>> +	 * Adjust frequency down in this case so that we don't pet
>> +	 * the watchdog too often.
>> +	 */
>> +	if (wdt->freq < 32768)
>> +		wdt->freq = wdt->freq * 9 / 10;
>> +
> 
> This seems broken: You are only adjusting the frequency value used by
> the driver. What has been programmed into the hardware already is still
> based on real frequency. Moreover, this path is not only taken when the
> watchdog is already running - but the latter case is what the subject
> and commit message suggests. I've noticed this by comparing
> bootloader-programmed values with those the driver assumes to see - 10%
> off, obviously.
> 
> So, what is actually supposed to happen here?
> 
> Jan
> 
>>  	pm_runtime_enable(dev);
>>  	ret = pm_runtime_get_sync(dev);
>>  	if (ret) {
>> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>  	wdd->min_timeout = 1;
>>  	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>>  		wdt->freq * 1000;
>> -	wdd->timeout = DEFAULT_HEARTBEAT;
>>  	wdd->parent = dev;
>>  
>> -	watchdog_init_timeout(wdd, heartbeat, dev);
>> -
>>  	watchdog_set_drvdata(wdd, wdt);
>>  	watchdog_set_nowayout(wdd, 1);
>>  	watchdog_set_restart_priority(wdd, 128);
>> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>  		goto err_iomap;
>>  	}
>>  
>> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
>> +		u32 time_left_ms;
>> +		u64 heartbeat_ms;
>> +		u32 wsize;
>> +
>> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
>> +		time_left_ms = rti_wdt_get_timeleft_ms(wdd);
>> +		heartbeat_ms = readl(wdt->base + RTIDWDPRLD);
>> +		heartbeat_ms <<= WDT_PRELOAD_SHIFT;
>> +		heartbeat_ms *= 1000;
>> +		do_div(heartbeat_ms, wdt->freq);
>> +		if (heartbeat_ms != heartbeat * 1000)
>> +			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
>> +
>> +		heartbeat = heartbeat_ms;
>> +		heartbeat /= 1000;
>> +
>> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);
>> +		ret = rti_wdt_setup_hw_hb(wdd, wsize);
>> +		if (ret) {
>> +			dev_err(dev, "bad window size.\n");
>> +			goto err_iomap;
>> +		}
>> +
>> +		last_ping = heartbeat_ms - time_left_ms;
>> +		if (time_left_ms > heartbeat_ms) {
>> +			dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n");
>> +			last_ping = 0;
>> +		}
>> +	}
>> +
>> +	watchdog_init_timeout(wdd, heartbeat, dev);
>> +
>>  	ret = watchdog_register_device(wdd);
>>  	if (ret) {
>>  		dev_err(dev, "cannot register watchdog device\n");
>>  		goto err_iomap;
>>  	}
>>  
>> +	if (last_ping)
>> +		watchdog_set_last_hw_keepalive(wdd, last_ping);
>> +
>>  	return 0;
>>  
>>  err_iomap:
> 

There is actually more "inaccurate". For now, I would try to address it 
like this:

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index 88815419ad1a..1b6629fa5bfc 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -231,14 +231,6 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/*
-	 * If watchdog is running at 32k clock, it is not accurate.
-	 * Adjust frequency down in this case so that we don't pet
-	 * the watchdog too often.
-	 */
-	if (wdt->freq < 32768)
-		wdt->freq = wdt->freq * 9 / 10;
-
 	pm_runtime_enable(dev);
 	ret = pm_runtime_get_sync(dev);
 	if (ret) {
@@ -252,8 +244,6 @@ static int rti_wdt_probe(struct platform_device *pdev)
 	wdd->info = &rti_wdt_info;
 	wdd->ops = &rti_wdt_ops;
 	wdd->min_timeout = 1;
-	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
-		wdt->freq * 1000;
 	wdd->parent = dev;
 
 	watchdog_set_drvdata(wdd, wdt);
@@ -280,7 +270,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		if (heartbeat_ms != heartbeat * 1000)
 			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
 
-		heartbeat = heartbeat_ms;
+		heartbeat = heartbeat_ms + 500;
 		heartbeat /= 1000;
 
 		wsize = readl(wdt->base + RTIWWDSIZECTRL);
@@ -297,6 +287,17 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		}
 	}
 
+	/*
+	 * If watchdog is running at 32k clock, it is not accurate.
+	 * Adjust frequency down in this case so that we don't pet
+	 * the watchdog too often.
+	 */
+	if (wdt->freq < 32768)
+		wdt->freq = wdt->freq * 9 / 10;
+
+	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
+		wdt->freq * 1000;
+
 	watchdog_init_timeout(wdd, heartbeat, dev);
 
 	ret = watchdog_register_device(wdd);


This moves the virtual frequency tweaking after reading back the 
programmed timeout. It also properly rounds that up to full seconds so 
that, e.g., bootloader-programmed programmed 60s will become 59s in the 
driver. That could have led to too short min_hw_heartbeat_ms.

I can send this out as real patch, but I'd still like to understand the 
freq fiddling first. Is that addressing a real hardware issue? Or was it 
actually papering over that round-up problem?

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe

[Nishanth Menon]

On 10:59-20220221, Jan Kiszka wrote:
> On 21.02.22 10:10, Jan Kiszka wrote:
> > On 17.07.20 15:29, Tero Kristo wrote:
> >> If the RTI watchdog is running already during probe, the driver must
> >> configure itself to match the HW. Window size and timeout is probed from
> >> hardware, and the last keepalive ping is adjusted to match it also.
> >>
> >> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> >> ---
> >>  drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++----
> >>  1 file changed, 102 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> >> index d456dd72d99a..7cbdc178ffe8 100644
> >> --- a/drivers/watchdog/rti_wdt.c
> >> +++ b/drivers/watchdog/rti_wdt.c
> >> @@ -35,7 +35,11 @@
> >>  
> >>  #define RTIWWDRX_NMI	0xa
> >>  
> >> -#define RTIWWDSIZE_50P	0x50
> >> +#define RTIWWDSIZE_50P		0x50
> >> +#define RTIWWDSIZE_25P		0x500
> >> +#define RTIWWDSIZE_12P5		0x5000
> >> +#define RTIWWDSIZE_6P25		0x50000
> >> +#define RTIWWDSIZE_3P125	0x500000
> >>  
> >>  #define WDENABLE_KEY	0xa98559da
> >>  
> >> @@ -48,7 +52,7 @@
> >>  
> >>  #define DWDST			BIT(1)
> >>  
> >> -static int heartbeat;
> >> +static int heartbeat = DEFAULT_HEARTBEAT;
> >>  
> >>  /*
> >>   * struct to hold data for each WDT device
> >> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
> >>  	 * be petted during the open window; not too early or not too late.
> >>  	 * The HW configuration options only allow for the open window size
> >>  	 * to be 50% or less than that; we obviouly want to configure the open
> >> -	 * window as large as possible so we select the 50% option. To avoid
> >> -	 * any glitches, we accommodate 5% safety margin also, so we setup
> >> -	 * the min_hw_hearbeat at 55% of the timeout period.
> >> +	 * window as large as possible so we select the 50% option.
> >>  	 */
> >> -	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
> >> +	wdd->min_hw_heartbeat_ms = 500 * wdd->timeout;
> >>  
> >>  	/* Generate NMI when wdt expires */
> >>  	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
> >> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd)
> >>  	return 0;
> >>  }
> >>  
> >> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
> >> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize)
> >> +{
> >> +	/*
> >> +	 * RTI only supports a windowed mode, where the watchdog can only
> >> +	 * be petted during the open window; not too early or not too late.
> >> +	 * The HW configuration options only allow for the open window size
> >> +	 * to be 50% or less than that.
> >> +	 */
> >> +	switch (wsize) {
> >> +	case RTIWWDSIZE_50P:
> >> +		/* 50% open window => 50% min heartbeat */
> >> +		wdd->min_hw_heartbeat_ms = 500 * heartbeat;
> >> +		break;
> >> +
> >> +	case RTIWWDSIZE_25P:
> >> +		/* 25% open window => 75% min heartbeat */
> >> +		wdd->min_hw_heartbeat_ms = 750 * heartbeat;
> >> +		break;
> >> +
> >> +	case RTIWWDSIZE_12P5:
> >> +		/* 12.5% open window => 87.5% min heartbeat */
> >> +		wdd->min_hw_heartbeat_ms = 875 * heartbeat;
> >> +		break;
> >> +
> >> +	case RTIWWDSIZE_6P25:
> >> +		/* 6.5% open window => 93.5% min heartbeat */
> >> +		wdd->min_hw_heartbeat_ms = 935 * heartbeat;
> >> +		break;
> >> +
> >> +	case RTIWWDSIZE_3P125:
> >> +		/* 3.125% open window => 96.9% min heartbeat */
> >> +		wdd->min_hw_heartbeat_ms = 969 * heartbeat;
> >> +		break;
> >> +
> >> +	default:
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd)
> >>  {
> >>  	u64 timer_counter;
> >>  	u32 val;
> >> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
> >>  
> >>  	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
> >>  
> >> +	timer_counter *= 1000;
> >> +
> >>  	do_div(timer_counter, wdt->freq);
> >>  
> >>  	return timer_counter;
> >>  }
> >>  
> >> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
> >> +{
> >> +	return rti_wdt_get_timeleft_ms(wdd) / 1000;
> >> +}
> >> +
> >>  static const struct watchdog_info rti_wdt_info = {
> >>  	.options = WDIOF_KEEPALIVEPING,
> >>  	.identity = "K3 RTI Watchdog",
> >> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
> >>  	struct watchdog_device *wdd;
> >>  	struct rti_wdt_device *wdt;
> >>  	struct clk *clk;
> >> +	u32 last_ping = 0;
> >>  
> >>  	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> >>  	if (!wdt)
> >> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev)
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> +	/*
> >> +	 * If watchdog is running at 32k clock, it is not accurate.
> >> +	 * Adjust frequency down in this case so that we don't pet
> >> +	 * the watchdog too often.
> >> +	 */
> >> +	if (wdt->freq < 32768)
> >> +		wdt->freq = wdt->freq * 9 / 10;
> >> +
> > 
> > This seems broken: You are only adjusting the frequency value used by
> > the driver. What has been programmed into the hardware already is still
> > based on real frequency. Moreover, this path is not only taken when the
> > watchdog is already running - but the latter case is what the subject
> > and commit message suggests. I've noticed this by comparing
> > bootloader-programmed values with those the driver assumes to see - 10%
> > off, obviously.
> > 
> > So, what is actually supposed to happen here?



This assumes that the clk is coming in from RC_OSC_32k - which is in the
range of accuracy of 10-20% off clock. also one more variable to keep in
mind is that the 32k divided clk from hfosc will not be exact.

Hari: Thoughts?

> > 
> > Jan
> > 
> >>  	pm_runtime_enable(dev);
> >>  	ret = pm_runtime_get_sync(dev);
> >>  	if (ret) {
> >> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
> >>  	wdd->min_timeout = 1;
> >>  	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
> >>  		wdt->freq * 1000;
> >> -	wdd->timeout = DEFAULT_HEARTBEAT;
> >>  	wdd->parent = dev;
> >>  
> >> -	watchdog_init_timeout(wdd, heartbeat, dev);
> >> -
> >>  	watchdog_set_drvdata(wdd, wdt);
> >>  	watchdog_set_nowayout(wdd, 1);
> >>  	watchdog_set_restart_priority(wdd, 128);
> >> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev)
> >>  		goto err_iomap;
> >>  	}
> >>  
> >> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
> >> +		u32 time_left_ms;
> >> +		u64 heartbeat_ms;
> >> +		u32 wsize;
> >> +
> >> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> >> +		time_left_ms = rti_wdt_get_timeleft_ms(wdd);
> >> +		heartbeat_ms = readl(wdt->base + RTIDWDPRLD);
> >> +		heartbeat_ms <<= WDT_PRELOAD_SHIFT;
> >> +		heartbeat_ms *= 1000;
> >> +		do_div(heartbeat_ms, wdt->freq);
> >> +		if (heartbeat_ms != heartbeat * 1000)
> >> +			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
> >> +
> >> +		heartbeat = heartbeat_ms;
> >> +		heartbeat /= 1000;
> >> +
> >> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);
> >> +		ret = rti_wdt_setup_hw_hb(wdd, wsize);
> >> +		if (ret) {
> >> +			dev_err(dev, "bad window size.\n");
> >> +			goto err_iomap;
> >> +		}
> >> +
> >> +		last_ping = heartbeat_ms - time_left_ms;
> >> +		if (time_left_ms > heartbeat_ms) {
> >> +			dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n");
> >> +			last_ping = 0;
> >> +		}
> >> +	}
> >> +
> >> +	watchdog_init_timeout(wdd, heartbeat, dev);
> >> +
> >>  	ret = watchdog_register_device(wdd);
> >>  	if (ret) {
> >>  		dev_err(dev, "cannot register watchdog device\n");
> >>  		goto err_iomap;
> >>  	}
> >>  
> >> +	if (last_ping)
> >> +		watchdog_set_last_hw_keepalive(wdd, last_ping);
> >> +
> >>  	return 0;
> >>  
> >>  err_iomap:
> > 
> 
> There is actually more "inaccurate". For now, I would try to address it 
> like this:
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index 88815419ad1a..1b6629fa5bfc 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -231,14 +231,6 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	/*
> -	 * If watchdog is running at 32k clock, it is not accurate.
> -	 * Adjust frequency down in this case so that we don't pet
> -	 * the watchdog too often.
> -	 */
> -	if (wdt->freq < 32768)
> -		wdt->freq = wdt->freq * 9 / 10;
> -
>  	pm_runtime_enable(dev);
>  	ret = pm_runtime_get_sync(dev);
>  	if (ret) {
> @@ -252,8 +244,6 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  	wdd->info = &rti_wdt_info;
>  	wdd->ops = &rti_wdt_ops;
>  	wdd->min_timeout = 1;
> -	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
> -		wdt->freq * 1000;
>  	wdd->parent = dev;
>  
>  	watchdog_set_drvdata(wdd, wdt);
> @@ -280,7 +270,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  		if (heartbeat_ms != heartbeat * 1000)
>  			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
>  
> -		heartbeat = heartbeat_ms;
> +		heartbeat = heartbeat_ms + 500;
>  		heartbeat /= 1000;
>  
>  		wsize = readl(wdt->base + RTIWWDSIZECTRL);
> @@ -297,6 +287,17 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/*
> +	 * If watchdog is running at 32k clock, it is not accurate.
> +	 * Adjust frequency down in this case so that we don't pet
> +	 * the watchdog too often.
> +	 */
> +	if (wdt->freq < 32768)
> +		wdt->freq = wdt->freq * 9 / 10;
> +
> +	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
> +		wdt->freq * 1000;
> +
>  	watchdog_init_timeout(wdd, heartbeat, dev);
>  
>  	ret = watchdog_register_device(wdd);
> 
> 
> This moves the virtual frequency tweaking after reading back the 
> programmed timeout. It also properly rounds that up to full seconds so 
> that, e.g., bootloader-programmed programmed 60s will become 59s in the 
> driver. That could have led to too short min_hw_heartbeat_ms.
> 
> I can send this out as real patch, but I'd still like to understand the 
> freq fiddling first. Is that addressing a real hardware issue? Or was it 
> actually papering over that round-up problem?
> 
> Jan
> 
> -- 
> Siemens AG, Technology
> Competence Center Embedded Linux

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D)/Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe

[Jan Kiszka]

On 21.02.22 13:44, Nishanth Menon wrote:
> On 10:59-20220221, Jan Kiszka wrote:
>> On 21.02.22 10:10, Jan Kiszka wrote:
>>> On 17.07.20 15:29, Tero Kristo wrote:
>>>> If the RTI watchdog is running already during probe, the driver must
>>>> configure itself to match the HW. Window size and timeout is probed from
>>>> hardware, and the last keepalive ping is adjusted to match it also.
>>>>
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>> ---
>>>>  drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++----
>>>>  1 file changed, 102 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>>>> index d456dd72d99a..7cbdc178ffe8 100644
>>>> --- a/drivers/watchdog/rti_wdt.c
>>>> +++ b/drivers/watchdog/rti_wdt.c
>>>> @@ -35,7 +35,11 @@
>>>>  
>>>>  #define RTIWWDRX_NMI	0xa
>>>>  
>>>> -#define RTIWWDSIZE_50P	0x50
>>>> +#define RTIWWDSIZE_50P		0x50
>>>> +#define RTIWWDSIZE_25P		0x500
>>>> +#define RTIWWDSIZE_12P5		0x5000
>>>> +#define RTIWWDSIZE_6P25		0x50000
>>>> +#define RTIWWDSIZE_3P125	0x500000
>>>>  
>>>>  #define WDENABLE_KEY	0xa98559da
>>>>  
>>>> @@ -48,7 +52,7 @@
>>>>  
>>>>  #define DWDST			BIT(1)
>>>>  
>>>> -static int heartbeat;
>>>> +static int heartbeat = DEFAULT_HEARTBEAT;
>>>>  
>>>>  /*
>>>>   * struct to hold data for each WDT device
>>>> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
>>>>  	 * be petted during the open window; not too early or not too late.
>>>>  	 * The HW configuration options only allow for the open window size
>>>>  	 * to be 50% or less than that; we obviouly want to configure the open
>>>> -	 * window as large as possible so we select the 50% option. To avoid
>>>> -	 * any glitches, we accommodate 5% safety margin also, so we setup
>>>> -	 * the min_hw_hearbeat at 55% of the timeout period.
>>>> +	 * window as large as possible so we select the 50% option.
>>>>  	 */
>>>> -	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
>>>> +	wdd->min_hw_heartbeat_ms = 500 * wdd->timeout;
>>>>  
>>>>  	/* Generate NMI when wdt expires */
>>>>  	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
>>>> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>>>> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize)
>>>> +{
>>>> +	/*
>>>> +	 * RTI only supports a windowed mode, where the watchdog can only
>>>> +	 * be petted during the open window; not too early or not too late.
>>>> +	 * The HW configuration options only allow for the open window size
>>>> +	 * to be 50% or less than that.
>>>> +	 */
>>>> +	switch (wsize) {
>>>> +	case RTIWWDSIZE_50P:
>>>> +		/* 50% open window => 50% min heartbeat */
>>>> +		wdd->min_hw_heartbeat_ms = 500 * heartbeat;
>>>> +		break;
>>>> +
>>>> +	case RTIWWDSIZE_25P:
>>>> +		/* 25% open window => 75% min heartbeat */
>>>> +		wdd->min_hw_heartbeat_ms = 750 * heartbeat;
>>>> +		break;
>>>> +
>>>> +	case RTIWWDSIZE_12P5:
>>>> +		/* 12.5% open window => 87.5% min heartbeat */
>>>> +		wdd->min_hw_heartbeat_ms = 875 * heartbeat;
>>>> +		break;
>>>> +
>>>> +	case RTIWWDSIZE_6P25:
>>>> +		/* 6.5% open window => 93.5% min heartbeat */
>>>> +		wdd->min_hw_heartbeat_ms = 935 * heartbeat;
>>>> +		break;
>>>> +
>>>> +	case RTIWWDSIZE_3P125:
>>>> +		/* 3.125% open window => 96.9% min heartbeat */
>>>> +		wdd->min_hw_heartbeat_ms = 969 * heartbeat;
>>>> +		break;
>>>> +
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd)
>>>>  {
>>>>  	u64 timer_counter;
>>>>  	u32 val;
>>>> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>>>>  
>>>>  	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
>>>>  
>>>> +	timer_counter *= 1000;
>>>> +
>>>>  	do_div(timer_counter, wdt->freq);
>>>>  
>>>>  	return timer_counter;
>>>>  }
>>>>  
>>>> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>>>> +{
>>>> +	return rti_wdt_get_timeleft_ms(wdd) / 1000;
>>>> +}
>>>> +
>>>>  static const struct watchdog_info rti_wdt_info = {
>>>>  	.options = WDIOF_KEEPALIVEPING,
>>>>  	.identity = "K3 RTI Watchdog",
>>>> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>>  	struct watchdog_device *wdd;
>>>>  	struct rti_wdt_device *wdt;
>>>>  	struct clk *clk;
>>>> +	u32 last_ping = 0;
>>>>  
>>>>  	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>>>  	if (!wdt)
>>>> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>>  		return -EINVAL;
>>>>  	}
>>>>  
>>>> +	/*
>>>> +	 * If watchdog is running at 32k clock, it is not accurate.
>>>> +	 * Adjust frequency down in this case so that we don't pet
>>>> +	 * the watchdog too often.
>>>> +	 */
>>>> +	if (wdt->freq < 32768)
>>>> +		wdt->freq = wdt->freq * 9 / 10;
>>>> +
>>>
>>> This seems broken: You are only adjusting the frequency value used by
>>> the driver. What has been programmed into the hardware already is still
>>> based on real frequency. Moreover, this path is not only taken when the
>>> watchdog is already running - but the latter case is what the subject
>>> and commit message suggests. I've noticed this by comparing
>>> bootloader-programmed values with those the driver assumes to see - 10%
>>> off, obviously.
>>>
>>> So, what is actually supposed to happen here?
> 
> 
> 
> This assumes that the clk is coming in from RC_OSC_32k - which is in the
> range of accuracy of 10-20% off clock. also one more variable to keep in
> mind is that the 32k divided clk from hfosc will not be exact.
> 

OK, so we do want a safety margin for min_hw_heartbeat_ms, make it
larger. But I still don't think it is best achieved by bending the
frequency. That will also affect other values, e.g. returning a wrong
programmed timeout to userspace if that was programmed earlier, using
the original frequency.

> Hari: Thoughts?
> 
>>>
>>> Jan
>>>
>>>>  	pm_runtime_enable(dev);
>>>>  	ret = pm_runtime_get_sync(dev);
>>>>  	if (ret) {
>>>> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>>  	wdd->min_timeout = 1;
>>>>  	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>>>>  		wdt->freq * 1000;
>>>> -	wdd->timeout = DEFAULT_HEARTBEAT;
>>>>  	wdd->parent = dev;
>>>>  
>>>> -	watchdog_init_timeout(wdd, heartbeat, dev);
>>>> -
>>>>  	watchdog_set_drvdata(wdd, wdt);
>>>>  	watchdog_set_nowayout(wdd, 1);
>>>>  	watchdog_set_restart_priority(wdd, 128);
>>>> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>>  		goto err_iomap;
>>>>  	}
>>>>  
>>>> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
>>>> +		u32 time_left_ms;
>>>> +		u64 heartbeat_ms;
>>>> +		u32 wsize;
>>>> +
>>>> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
>>>> +		time_left_ms = rti_wdt_get_timeleft_ms(wdd);
>>>> +		heartbeat_ms = readl(wdt->base + RTIDWDPRLD);
>>>> +		heartbeat_ms <<= WDT_PRELOAD_SHIFT;
>>>> +		heartbeat_ms *= 1000;
>>>> +		do_div(heartbeat_ms, wdt->freq);
>>>> +		if (heartbeat_ms != heartbeat * 1000)
>>>> +			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
>>>> +
>>>> +		heartbeat = heartbeat_ms;
>>>> +		heartbeat /= 1000;
>>>> +
>>>> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);
>>>> +		ret = rti_wdt_setup_hw_hb(wdd, wsize);
>>>> +		if (ret) {
>>>> +			dev_err(dev, "bad window size.\n");
>>>> +			goto err_iomap;
>>>> +		}
>>>> +
>>>> +		last_ping = heartbeat_ms - time_left_ms;
>>>> +		if (time_left_ms > heartbeat_ms) {
>>>> +			dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n");
>>>> +			last_ping = 0;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	watchdog_init_timeout(wdd, heartbeat, dev);
>>>> +
>>>>  	ret = watchdog_register_device(wdd);
>>>>  	if (ret) {
>>>>  		dev_err(dev, "cannot register watchdog device\n");
>>>>  		goto err_iomap;
>>>>  	}
>>>>  
>>>> +	if (last_ping)
>>>> +		watchdog_set_last_hw_keepalive(wdd, last_ping);
>>>> +
>>>>  	return 0;
>>>>  
>>>>  err_iomap:
>>>
>>
>> There is actually more "inaccurate". For now, I would try to address it 
>> like this:
>>
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> index 88815419ad1a..1b6629fa5bfc 100644
>> --- a/drivers/watchdog/rti_wdt.c
>> +++ b/drivers/watchdog/rti_wdt.c
>> @@ -231,14 +231,6 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>  		return -EINVAL;
>>  	}
>>  
>> -	/*
>> -	 * If watchdog is running at 32k clock, it is not accurate.
>> -	 * Adjust frequency down in this case so that we don't pet
>> -	 * the watchdog too often.
>> -	 */
>> -	if (wdt->freq < 32768)
>> -		wdt->freq = wdt->freq * 9 / 10;
>> -
>>  	pm_runtime_enable(dev);
>>  	ret = pm_runtime_get_sync(dev);
>>  	if (ret) {
>> @@ -252,8 +244,6 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>  	wdd->info = &rti_wdt_info;
>>  	wdd->ops = &rti_wdt_ops;
>>  	wdd->min_timeout = 1;
>> -	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>> -		wdt->freq * 1000;
>>  	wdd->parent = dev;
>>  
>>  	watchdog_set_drvdata(wdd, wdt);
>> @@ -280,7 +270,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>  		if (heartbeat_ms != heartbeat * 1000)
>>  			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
>>  
>> -		heartbeat = heartbeat_ms;
>> +		heartbeat = heartbeat_ms + 500;
>>  		heartbeat /= 1000;
>>  
>>  		wsize = readl(wdt->base + RTIWWDSIZECTRL);
>> @@ -297,6 +287,17 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>  		}
>>  	}
>>  
>> +	/*
>> +	 * If watchdog is running at 32k clock, it is not accurate.
>> +	 * Adjust frequency down in this case so that we don't pet
>> +	 * the watchdog too often.
>> +	 */
>> +	if (wdt->freq < 32768)
>> +		wdt->freq = wdt->freq * 9 / 10;
>> +
>> +	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>> +		wdt->freq * 1000;
>> +
>>  	watchdog_init_timeout(wdd, heartbeat, dev);
>>  
>>  	ret = watchdog_register_device(wdd);
>>
>>
>> This moves the virtual frequency tweaking after reading back the 
>> programmed timeout. It also properly rounds that up to full seconds so 
>> that, e.g., bootloader-programmed programmed 60s will become 59s in the 
>> driver. That could have led to too short min_hw_heartbeat_ms.
>>
>> I can send this out as real patch, but I'd still like to understand the 
>> freq fiddling first. Is that addressing a real hardware issue? Or was it 
>> actually papering over that round-up problem?
>>
>> Jan
>>
>> -- 
>> Siemens AG, Technology
>> Competence Center Embedded Linux
> 

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe

[Jan Kiszka]

On 21.02.22 13:53, Jan Kiszka wrote:
> On 21.02.22 13:44, Nishanth Menon wrote:
>> On 10:59-20220221, Jan Kiszka wrote:
>>> On 21.02.22 10:10, Jan Kiszka wrote:
>>>> On 17.07.20 15:29, Tero Kristo wrote:
>>>>> If the RTI watchdog is running already during probe, the driver must
>>>>> configure itself to match the HW. Window size and timeout is probed from
>>>>> hardware, and the last keepalive ping is adjusted to match it also.
>>>>>
>>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>>> ---
>>>>>  drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++----
>>>>>  1 file changed, 102 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>>>>> index d456dd72d99a..7cbdc178ffe8 100644
>>>>> --- a/drivers/watchdog/rti_wdt.c
>>>>> +++ b/drivers/watchdog/rti_wdt.c
>>>>> @@ -35,7 +35,11 @@
>>>>>  
>>>>>  #define RTIWWDRX_NMI	0xa
>>>>>  
>>>>> -#define RTIWWDSIZE_50P	0x50
>>>>> +#define RTIWWDSIZE_50P		0x50
>>>>> +#define RTIWWDSIZE_25P		0x500
>>>>> +#define RTIWWDSIZE_12P5		0x5000
>>>>> +#define RTIWWDSIZE_6P25		0x50000
>>>>> +#define RTIWWDSIZE_3P125	0x500000
>>>>>  
>>>>>  #define WDENABLE_KEY	0xa98559da
>>>>>  
>>>>> @@ -48,7 +52,7 @@
>>>>>  
>>>>>  #define DWDST			BIT(1)
>>>>>  
>>>>> -static int heartbeat;
>>>>> +static int heartbeat = DEFAULT_HEARTBEAT;
>>>>>  
>>>>>  /*
>>>>>   * struct to hold data for each WDT device
>>>>> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd)
>>>>>  	 * be petted during the open window; not too early or not too late.
>>>>>  	 * The HW configuration options only allow for the open window size
>>>>>  	 * to be 50% or less than that; we obviouly want to configure the open
>>>>> -	 * window as large as possible so we select the 50% option. To avoid
>>>>> -	 * any glitches, we accommodate 5% safety margin also, so we setup
>>>>> -	 * the min_hw_hearbeat at 55% of the timeout period.
>>>>> +	 * window as large as possible so we select the 50% option.
>>>>>  	 */
>>>>> -	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
>>>>> +	wdd->min_hw_heartbeat_ms = 500 * wdd->timeout;
>>>>>  
>>>>>  	/* Generate NMI when wdt expires */
>>>>>  	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
>>>>> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd)
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>>>>> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize)
>>>>> +{
>>>>> +	/*
>>>>> +	 * RTI only supports a windowed mode, where the watchdog can only
>>>>> +	 * be petted during the open window; not too early or not too late.
>>>>> +	 * The HW configuration options only allow for the open window size
>>>>> +	 * to be 50% or less than that.
>>>>> +	 */
>>>>> +	switch (wsize) {
>>>>> +	case RTIWWDSIZE_50P:
>>>>> +		/* 50% open window => 50% min heartbeat */
>>>>> +		wdd->min_hw_heartbeat_ms = 500 * heartbeat;
>>>>> +		break;
>>>>> +
>>>>> +	case RTIWWDSIZE_25P:
>>>>> +		/* 25% open window => 75% min heartbeat */
>>>>> +		wdd->min_hw_heartbeat_ms = 750 * heartbeat;
>>>>> +		break;
>>>>> +
>>>>> +	case RTIWWDSIZE_12P5:
>>>>> +		/* 12.5% open window => 87.5% min heartbeat */
>>>>> +		wdd->min_hw_heartbeat_ms = 875 * heartbeat;
>>>>> +		break;
>>>>> +
>>>>> +	case RTIWWDSIZE_6P25:
>>>>> +		/* 6.5% open window => 93.5% min heartbeat */
>>>>> +		wdd->min_hw_heartbeat_ms = 935 * heartbeat;
>>>>> +		break;
>>>>> +
>>>>> +	case RTIWWDSIZE_3P125:
>>>>> +		/* 3.125% open window => 96.9% min heartbeat */
>>>>> +		wdd->min_hw_heartbeat_ms = 969 * heartbeat;
>>>>> +		break;
>>>>> +
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd)
>>>>>  {
>>>>>  	u64 timer_counter;
>>>>>  	u32 val;
>>>>> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>>>>>  
>>>>>  	timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR);
>>>>>  
>>>>> +	timer_counter *= 1000;
>>>>> +
>>>>>  	do_div(timer_counter, wdt->freq);
>>>>>  
>>>>>  	return timer_counter;
>>>>>  }
>>>>>  
>>>>> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd)
>>>>> +{
>>>>> +	return rti_wdt_get_timeleft_ms(wdd) / 1000;
>>>>> +}
>>>>> +
>>>>>  static const struct watchdog_info rti_wdt_info = {
>>>>>  	.options = WDIOF_KEEPALIVEPING,
>>>>>  	.identity = "K3 RTI Watchdog",
>>>>> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>>>  	struct watchdog_device *wdd;
>>>>>  	struct rti_wdt_device *wdt;
>>>>>  	struct clk *clk;
>>>>> +	u32 last_ping = 0;
>>>>>  
>>>>>  	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
>>>>>  	if (!wdt)
>>>>> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>>>  		return -EINVAL;
>>>>>  	}
>>>>>  
>>>>> +	/*
>>>>> +	 * If watchdog is running at 32k clock, it is not accurate.
>>>>> +	 * Adjust frequency down in this case so that we don't pet
>>>>> +	 * the watchdog too often.
>>>>> +	 */
>>>>> +	if (wdt->freq < 32768)
>>>>> +		wdt->freq = wdt->freq * 9 / 10;
>>>>> +
>>>>
>>>> This seems broken: You are only adjusting the frequency value used by
>>>> the driver. What has been programmed into the hardware already is still
>>>> based on real frequency. Moreover, this path is not only taken when the
>>>> watchdog is already running - but the latter case is what the subject
>>>> and commit message suggests. I've noticed this by comparing
>>>> bootloader-programmed values with those the driver assumes to see - 10%
>>>> off, obviously.
>>>>
>>>> So, what is actually supposed to happen here?
>>
>>
>>
>> This assumes that the clk is coming in from RC_OSC_32k - which is in the
>> range of accuracy of 10-20% off clock. also one more variable to keep in
>> mind is that the 32k divided clk from hfosc will not be exact.
>>
> 
> OK, so we do want a safety margin for min_hw_heartbeat_ms, make it
> larger. But I still don't think it is best achieved by bending the
> frequency. That will also affect other values, e.g. returning a wrong
> programmed timeout to userspace if that was programmed earlier, using
> the original frequency.
> 

I think I'm starting to get the original logic, and the result now works
here:

The clock driving the watchdog might be slower than thought, and then we
may time out later than intended - generally not an issue. But it may
also be faster, and then we will see an expiry earlier than what is
supposed to be configured via "heartbeat". For the latter case, we lower
the frequency virtually by 10%, crossing fingers that this is enough.

The problems are now:
 - U-Boot (as a known early watchdog starter) does not do that as well,
   and we will cause at least confusion on Linux side (60s will become
   66s from Linux POV e.g., and we may expire at 54s already)
    => U-Boot should add the same 10%, patch will be sent
 - even with U-Boot on the same page, the rounding issue will prevent
   accurate calculations of derived values, namely min_hw_heartbeat_ms.
    => patch to come
 - and ...

>> Hari: Thoughts?
>>
>>>>
>>>> Jan
>>>>
>>>>>  	pm_runtime_enable(dev);
>>>>>  	ret = pm_runtime_get_sync(dev);
>>>>>  	if (ret) {
>>>>> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>>>  	wdd->min_timeout = 1;
>>>>>  	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>>>>>  		wdt->freq * 1000;
>>>>> -	wdd->timeout = DEFAULT_HEARTBEAT;
>>>>>  	wdd->parent = dev;
>>>>>  
>>>>> -	watchdog_init_timeout(wdd, heartbeat, dev);
>>>>> -
>>>>>  	watchdog_set_drvdata(wdd, wdt);
>>>>>  	watchdog_set_nowayout(wdd, 1);
>>>>>  	watchdog_set_restart_priority(wdd, 128);
>>>>> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>>>  		goto err_iomap;
>>>>>  	}
>>>>>  
>>>>> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
>>>>> +		u32 time_left_ms;
>>>>> +		u64 heartbeat_ms;
>>>>> +		u32 wsize;
>>>>> +
>>>>> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
>>>>> +		time_left_ms = rti_wdt_get_timeleft_ms(wdd);
>>>>> +		heartbeat_ms = readl(wdt->base + RTIDWDPRLD);
>>>>> +		heartbeat_ms <<= WDT_PRELOAD_SHIFT;
>>>>> +		heartbeat_ms *= 1000;
>>>>> +		do_div(heartbeat_ms, wdt->freq);
>>>>> +		if (heartbeat_ms != heartbeat * 1000)
>>>>> +			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");

...this comparison is not ok, leading to a practically impossible to
fulfill condition. Will fix that up while adding the round-up.

Jan

>>>>> +
>>>>> +		heartbeat = heartbeat_ms;
>>>>> +		heartbeat /= 1000;
>>>>> +
>>>>> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);
>>>>> +		ret = rti_wdt_setup_hw_hb(wdd, wsize);
>>>>> +		if (ret) {
>>>>> +			dev_err(dev, "bad window size.\n");
>>>>> +			goto err_iomap;
>>>>> +		}
>>>>> +
>>>>> +		last_ping = heartbeat_ms - time_left_ms;
>>>>> +		if (time_left_ms > heartbeat_ms) {
>>>>> +			dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n");
>>>>> +			last_ping = 0;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	watchdog_init_timeout(wdd, heartbeat, dev);
>>>>> +
>>>>>  	ret = watchdog_register_device(wdd);
>>>>>  	if (ret) {
>>>>>  		dev_err(dev, "cannot register watchdog device\n");
>>>>>  		goto err_iomap;
>>>>>  	}
>>>>>  
>>>>> +	if (last_ping)
>>>>> +		watchdog_set_last_hw_keepalive(wdd, last_ping);
>>>>> +
>>>>>  	return 0;
>>>>>  
>>>>>  err_iomap:
>>>>
>>>
>>> There is actually more "inaccurate". For now, I would try to address it 
>>> like this:
>>>
>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>>> index 88815419ad1a..1b6629fa5bfc 100644
>>> --- a/drivers/watchdog/rti_wdt.c
>>> +++ b/drivers/watchdog/rti_wdt.c
>>> @@ -231,14 +231,6 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	/*
>>> -	 * If watchdog is running at 32k clock, it is not accurate.
>>> -	 * Adjust frequency down in this case so that we don't pet
>>> -	 * the watchdog too often.
>>> -	 */
>>> -	if (wdt->freq < 32768)
>>> -		wdt->freq = wdt->freq * 9 / 10;
>>> -
>>>  	pm_runtime_enable(dev);
>>>  	ret = pm_runtime_get_sync(dev);
>>>  	if (ret) {
>>> @@ -252,8 +244,6 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>  	wdd->info = &rti_wdt_info;
>>>  	wdd->ops = &rti_wdt_ops;
>>>  	wdd->min_timeout = 1;
>>> -	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>>> -		wdt->freq * 1000;
>>>  	wdd->parent = dev;
>>>  
>>>  	watchdog_set_drvdata(wdd, wdt);
>>> @@ -280,7 +270,7 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>  		if (heartbeat_ms != heartbeat * 1000)
>>>  			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
>>>  
>>> -		heartbeat = heartbeat_ms;
>>> +		heartbeat = heartbeat_ms + 500;
>>>  		heartbeat /= 1000;
>>>  
>>>  		wsize = readl(wdt->base + RTIWWDSIZECTRL);
>>> @@ -297,6 +287,17 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>>  		}
>>>  	}
>>>  
>>> +	/*
>>> +	 * If watchdog is running at 32k clock, it is not accurate.
>>> +	 * Adjust frequency down in this case so that we don't pet
>>> +	 * the watchdog too often.
>>> +	 */
>>> +	if (wdt->freq < 32768)
>>> +		wdt->freq = wdt->freq * 9 / 10;
>>> +
>>> +	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
>>> +		wdt->freq * 1000;
>>> +
>>>  	watchdog_init_timeout(wdd, heartbeat, dev);
>>>  
>>>  	ret = watchdog_register_device(wdd);
>>>
>>>
>>> This moves the virtual frequency tweaking after reading back the 
>>> programmed timeout. It also properly rounds that up to full seconds so 
>>> that, e.g., bootloader-programmed programmed 60s will become 59s in the 
>>> driver. That could have led to too short min_hw_heartbeat_ms.
>>>
>>> I can send this out as real patch, but I'd still like to understand the 
>>> freq fiddling first. Is that addressing a real hardware issue? Or was it 
>>> actually papering over that round-up problem?
>>>
>>> Jan
>>>
>>> -- 
>>> Siemens AG, Technology
>>> Competence Center Embedded Linux
>>
> 
> Jan
> 

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe

[Hari Nagalla]

On 2/21/22 10:03, Jan Kiszka wrote:
>> K, so we do want a safety margin for min_hw_heartbeat_ms, make it
>> larger. But I still don't think it is best achieved by bending the
>> frequency. That will also affect other values, e.g. returning a wrong
>> programmed timeout to userspace if that was programmed earlier, using
>> the original frequency.
>>
> I think I'm starting to get the original logic, and the result now works
> here:
> 
> The clock driving the watchdog might be slower than thought, and then we
> may time out later than intended - generally not an issue. But it may
> also be faster, and then we will see an expiry earlier than what is
> supposed to be configured via "heartbeat". For the latter case, we lower
> the frequency virtually by 10%, crossing fingers that this is enough.
> 
Humm.. To me it appears the intent is to adjust when the input 32KHz 
clock is slower? when it is slower we reduce the pulse count by 10% 
(assuming the crystals are with 10% off clock) so that the desired 
timeout is achieved with lesser pulse count?
> The problems are now:
>   - U-Boot (as a known early watchdog starter) does not do that as well,
>     and we will cause at least confusion on Linux side (60s will become
>     66s from Linux POV e.g., and we may expire at 54s already)
>      => U-Boot should add the same 10%, patch will be sent
Yes, i see that we need similar adjustment in u-boot as well.
>   - even with U-Boot on the same page, the rounding issue will prevent
>     accurate calculations of derived values, namely min_hw_heartbeat_ms.
>      => patch to come
>   - and ...
>