[PATCHv2] watchdog: rti-wdt: support attaching to running wdt

[PATCHv2] watchdog: rti-wdt: support attaching to running wdt

[Tero Kristo]

Hi,

Version 2 of the series has quite a few changes based on feedback on
previous version.

1) Add new watchdog core API for adjusting the last hw keepalive time
   (Patch #2)
2) Modify the driver to support adjusting the window size, current
   driver only supports 50% window size. The window size is configured
   via a module parameter.
3) Modify the attach mechanism to running wdt to configure the heartbeat
   and window size based on running HW wdt setup.
4) Fix a runtime PM issue that was noticed while dealing with the new
   module parameter (tested via rmmod / modprobe)

-Tero


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

[PATCHv2 1/5] 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

[PATCHv2 2/5] 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>
---
 drivers/watchdog/watchdog_dev.c | 23 +++++++++++++++++++++++
 include/linux/watchdog.h        |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index bc1cfa288553..5848551cf29d 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -1138,6 +1138,29 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)
 	watchdog_cdev_unregister(wdd);
 }
 
+/*
+ *	watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog
+ *
+ *	Adjusts the last known HW keepalive time for a watchdog timer.
+ *	This is needed in case where watchdog has been started before
+ *	kernel by someone like bootloader, and it can't be pinged
+ *	immediately. This adjusts the watchdog ping period to match
+ *	the currently running timer.
+ */
+int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
+				   unsigned int last_ping_ms)
+{
+	struct watchdog_core_data *wd_data = wdd->wd_data;
+	ktime_t now;
+
+	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

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

[Guenter Roeck]

On 7/3/20 5:04 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>
> ---
>  drivers/watchdog/watchdog_dev.c | 23 +++++++++++++++++++++++
>  include/linux/watchdog.h        |  2 ++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index bc1cfa288553..5848551cf29d 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -1138,6 +1138,29 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)
>  	watchdog_cdev_unregister(wdd);
>  }
>  
> +/*
> + *	watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog
> + *
> + *	Adjusts the last known HW keepalive time for a watchdog timer.
> + *	This is needed in case where watchdog has been started before
> + *	kernel by someone like bootloader, and it can't be pinged

... needed if the watchdog is already running when the probe function
is called, and ...

> + *	immediately. This adjusts the watchdog ping period to match
> + *	the currently running timer.

It doesn't adjust the ping period.

> + */

last_ping_ms needs to be documented (the last heartbeat was last_ping_ms
milliseconds ago ?), both here and in Documentation/watchdog/watchdog-kernel-api.rst.
It needs to be documented that the function must be called immediately
after watchdog registration, and that min_hw_heartbeat_ms must
be set for it to be useful.

> +int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
> +				   unsigned int last_ping_ms)
> +{
> +	struct watchdog_core_data *wd_data = wdd->wd_data;

This needs a NULL check, in case it is called before watchdog driver
registration.

> +	ktime_t now;
> +
> +	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: [PATCHv2 2/5] watchdog: add support for adjusting last known HW keepalive time

[Tero Kristo]

On 05/07/2020 17:58, Guenter Roeck wrote:
> On 7/3/20 5:04 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>
>> ---
>>   drivers/watchdog/watchdog_dev.c | 23 +++++++++++++++++++++++
>>   include/linux/watchdog.h        |  2 ++
>>   2 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index bc1cfa288553..5848551cf29d 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -1138,6 +1138,29 @@ void watchdog_dev_unregister(struct watchdog_device *wdd)
>>   	watchdog_cdev_unregister(wdd);
>>   }
>>   
>> +/*
>> + *	watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog
>> + *
>> + *	Adjusts the last known HW keepalive time for a watchdog timer.
>> + *	This is needed in case where watchdog has been started before
>> + *	kernel by someone like bootloader, and it can't be pinged
> 
> ... needed if the watchdog is already running when the probe function
> is called, and ...
> 
>> + *	immediately. This adjusts the watchdog ping period to match
>> + *	the currently running timer.
> 
> It doesn't adjust the ping period.
> 
>> + */
> 
> last_ping_ms needs to be documented (the last heartbeat was last_ping_ms
> milliseconds ago ?), both here and in Documentation/watchdog/watchdog-kernel-api.rst.
> It needs to be documented that the function must be called immediately
> after watchdog registration, and that min_hw_heartbeat_ms must
> be set for it to be useful.
> 
>> +int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd,
>> +				   unsigned int last_ping_ms)
>> +{
>> +	struct watchdog_core_data *wd_data = wdd->wd_data;
> 
> This needs a NULL check, in case it is called before watchdog driver
> registration.

Ok will fix all the above in next revision.

-Tero

> 
>> +	ktime_t now;
>> +
>> +	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 *);
>>   
>>
> 

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

[PATCHv2 3/5] watchdog: rti-wdt: add support for window size configuration

[Tero Kristo]

RTI watchdog can support different open window sizes. Add support for
these and add a new module parameter to configure it. The default open
window size for the driver still remains at 50%.

Also, modify the margin calculation logic a bit for 32k source clock,
instead of adding a margin to every window config, assume the 32k source
clock is running slower.

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

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index d456dd72d99a..110bfc8d0bb3 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -19,7 +19,8 @@
 #include <linux/types.h>
 #include <linux/watchdog.h>
 
-#define DEFAULT_HEARTBEAT 60
+#define DEFAULT_HEARTBEAT	60
+#define DEFAULT_WINDOWSIZE	50
 
 /* Max heartbeat is calculated at 32kHz source clock */
 #define MAX_HEARTBEAT	1000
@@ -35,9 +36,13 @@
 
 #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
+#define WDENABLE_KEY		0xa98559da
 
 #define WDKEY_SEQ0		0xe51a
 #define WDKEY_SEQ1		0xa35c
@@ -48,7 +53,8 @@
 
 #define DWDST			BIT(1)
 
-static int heartbeat;
+static int heartbeat =		DEFAULT_HEARTBEAT;
+static u32 wsize =		DEFAULT_WINDOWSIZE;
 
 /*
  * struct to hold data for each WDT device
@@ -62,34 +68,93 @@ struct rti_wdt_device {
 	struct watchdog_device	wdd;
 };
 
+static int rti_wdt_convert_wsize(void)
+{
+	if (wsize >= 50) {
+		wsize = RTIWWDSIZE_50P;
+	} else if (wsize >= 25) {
+		wsize = RTIWWDSIZE_25P;
+	} else if (wsize > 12) {
+		wsize = RTIWWDSIZE_12P5;
+	} else if (wsize > 6) {
+		wsize = RTIWWDSIZE_6P25;
+	} else if (wsize > 3) {
+		wsize = RTIWWDSIZE_3P125;
+	} else {
+		pr_err("%s: bad windowsize: %d\n", __func__, wsize);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd)
+{
+	/*
+	 * 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:
+		pr_err("%s: Bad watchdog window size!\n", __func__);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int rti_wdt_start(struct watchdog_device *wdd)
 {
 	u32 timer_margin;
 	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
+	int ret;
 
 	/* set timeout period */
-	timer_margin = (u64)wdd->timeout * wdt->freq;
+	timer_margin = (u64)heartbeat * wdt->freq;
 	timer_margin >>= WDT_PRELOAD_SHIFT;
 	if (timer_margin > WDT_PRELOAD_MAX)
 		timer_margin = WDT_PRELOAD_MAX;
 	writel_relaxed(timer_margin, wdt->base + RTIDWDPRLD);
 
-	/*
-	 * 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; 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.
-	 */
-	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
+	ret = rti_wdt_convert_wsize();
+	if (ret)
+		return ret;
+
+	ret = rti_wdt_setup_hw_hb(wdd);
+	if (ret)
+		return ret;
 
 	/* Generate NMI when wdt expires */
 	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
 
-	/* Open window size 50%; this is the largest window size available */
-	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
+	writel_relaxed(wsize, wdt->base + RTIWWDSIZECTRL);
 
 	readl_relaxed(wdt->base + RTIWWDSIZECTRL);
 
@@ -169,6 +234,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 > 30000 && wdt->freq < 32768)
+		wdt->freq = 30000;
+
 	pm_runtime_enable(dev);
 	ret = pm_runtime_get_sync(dev);
 	if (ret) {
@@ -251,5 +324,10 @@ MODULE_PARM_DESC(heartbeat,
 		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
 		 __MODULE_STRING(DEFAULT_HEARTBEAT));
 
+module_param(wsize, uint, 0);
+MODULE_PARM_DESC(wsize,
+		 "Watchdog open window size in percentage from 3 to 50, "
+		 "default " __MODULE_STRING(DEFAULT_WINDOW_SIZE));
+
 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: [PATCHv2 3/5] watchdog: rti-wdt: add support for window size configuration

[Guenter Roeck]

On 7/3/20 5:04 AM, Tero Kristo wrote:
> RTI watchdog can support different open window sizes. Add support for
> these and add a new module parameter to configure it. The default open
> window size for the driver still remains at 50%.
> 
> Also, modify the margin calculation logic a bit for 32k source clock,
> instead of adding a margin to every window config, assume the 32k source
> clock is running slower.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++------
>  1 file changed, 95 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index d456dd72d99a..110bfc8d0bb3 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -19,7 +19,8 @@
>  #include <linux/types.h>
>  #include <linux/watchdog.h>
>  
> -#define DEFAULT_HEARTBEAT 60
> +#define DEFAULT_HEARTBEAT	60
> +#define DEFAULT_WINDOWSIZE	50
>  
>  /* Max heartbeat is calculated at 32kHz source clock */
>  #define MAX_HEARTBEAT	1000
> @@ -35,9 +36,13 @@
>  
>  #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
> +#define WDENABLE_KEY		0xa98559da
>  
>  #define WDKEY_SEQ0		0xe51a
>  #define WDKEY_SEQ1		0xa35c
> @@ -48,7 +53,8 @@
>  
>  #define DWDST			BIT(1)
>  
> -static int heartbeat;
> +static int heartbeat =		DEFAULT_HEARTBEAT;
> +static u32 wsize =		DEFAULT_WINDOWSIZE;
>  
>  /*
>   * struct to hold data for each WDT device
> @@ -62,34 +68,93 @@ struct rti_wdt_device {
>  	struct watchdog_device	wdd;
>  };
>  
> +static int rti_wdt_convert_wsize(void)
> +{
> +	if (wsize >= 50) {
> +		wsize = RTIWWDSIZE_50P;
> +	} else if (wsize >= 25) {
> +		wsize = RTIWWDSIZE_25P;
> +	} else if (wsize > 12) {
> +		wsize = RTIWWDSIZE_12P5;
> +	} else if (wsize > 6) {
> +		wsize = RTIWWDSIZE_6P25;
> +	} else if (wsize > 3) {
> +		wsize = RTIWWDSIZE_3P125;
> +	} else {
> +		pr_err("%s: bad windowsize: %d\n", __func__, wsize);

Please do not use pr_ functions. Pass the watchdog device as argument
and use dev_err().

Also, this function modifies the wsize parameter. When called
again, that parameter will have a  totally different meaning, and
the second call to this function will always set the window size
to 50.

On top of all that, window sizes larger than 50 are set to 50,
window sizes between 4 and 49 are adjusted, and window sizes <= 3
are rejected. That is not exactly consistent.

Does this module parameter really add value / make sense ?
What is the use case ? We should not add such complexity without
use case.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd)
> +{
> +	/*
> +	 * 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:
> +		pr_err("%s: Bad watchdog window size!\n", __func__);

Same here.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int rti_wdt_start(struct watchdog_device *wdd)
>  {
>  	u32 timer_margin;
>  	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
> +	int ret;
>  
>  	/* set timeout period */
> -	timer_margin = (u64)wdd->timeout * wdt->freq;
> +	timer_margin = (u64)heartbeat * wdt->freq;
>  	timer_margin >>= WDT_PRELOAD_SHIFT;
>  	if (timer_margin > WDT_PRELOAD_MAX)
>  		timer_margin = WDT_PRELOAD_MAX;
>  	writel_relaxed(timer_margin, wdt->base + RTIDWDPRLD);
>  
> -	/*
> -	 * 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; 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.
> -	 */
> -	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
> +	ret = rti_wdt_convert_wsize();
> +	if (ret)
> +		return ret;
> +
> +	ret = rti_wdt_setup_hw_hb(wdd);
> +	if (ret)
> +		return ret;
>  

This is the wrong place to validate the window size. It should be done
only once, in the probe function. The start function should not fail
because of a bad window size.

With such parameters, the wsize written into the chip should be kept
in struct rti_wdt_device if it needs to be set more than once.
The module parameter should not be changed, and it should not be used
to store the register value. min_hw_heartbeat_ms needs to be set in the
probe function, not in the start function. Sorry that I didn't notice
that before.

>  	/* Generate NMI when wdt expires */
>  	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
>  
> -	/* Open window size 50%; this is the largest window size available */
> -	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
> +	writel_relaxed(wsize, wdt->base + RTIWWDSIZECTRL);
>  
>  	readl_relaxed(wdt->base + RTIWWDSIZECTRL);
>  
> @@ -169,6 +234,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 > 30000 && wdt->freq < 32768)
> +		wdt->freq = 30000;
> +

Combining that with a window size of 96.9% min heartbeat is asking
for trouble. It will be all but impossible to catch the window with
such constraints if the frequency is really that inaccurate.

>  	pm_runtime_enable(dev);
>  	ret = pm_runtime_get_sync(dev);
>  	if (ret) {
> @@ -251,5 +324,10 @@ MODULE_PARM_DESC(heartbeat,
>  		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
>  		 __MODULE_STRING(DEFAULT_HEARTBEAT));
>  
> +module_param(wsize, uint, 0);
> +MODULE_PARM_DESC(wsize,
> +		 "Watchdog open window size in percentage from 3 to 50, "
> +		 "default " __MODULE_STRING(DEFAULT_WINDOW_SIZE));
> +
>  MODULE_LICENSE("GPL");
>  MODULE_ALIAS("platform:rti-wdt");
> 

Re: [PATCHv2 3/5] watchdog: rti-wdt: add support for window size configuration

[Tero Kristo]

On 05/07/2020 17:49, Guenter Roeck wrote:
> On 7/3/20 5:04 AM, Tero Kristo wrote:
>> RTI watchdog can support different open window sizes. Add support for
>> these and add a new module parameter to configure it. The default open
>> window size for the driver still remains at 50%.
>>
>> Also, modify the margin calculation logic a bit for 32k source clock,
>> instead of adding a margin to every window config, assume the 32k source
>> clock is running slower.

I'll drop this patch mostly in next rev to get rid of the dynamic config 
for window size and always use 50%. I will just read the window size in 
case someone has started the watchdog beforehand.

>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++------
>>   1 file changed, 95 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> index d456dd72d99a..110bfc8d0bb3 100644
>> --- a/drivers/watchdog/rti_wdt.c
>> +++ b/drivers/watchdog/rti_wdt.c
>> @@ -19,7 +19,8 @@
>>   #include <linux/types.h>
>>   #include <linux/watchdog.h>
>>   
>> -#define DEFAULT_HEARTBEAT 60
>> +#define DEFAULT_HEARTBEAT	60
>> +#define DEFAULT_WINDOWSIZE	50
>>   
>>   /* Max heartbeat is calculated at 32kHz source clock */
>>   #define MAX_HEARTBEAT	1000
>> @@ -35,9 +36,13 @@
>>   
>>   #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
>> +#define WDENABLE_KEY		0xa98559da
>>   
>>   #define WDKEY_SEQ0		0xe51a
>>   #define WDKEY_SEQ1		0xa35c
>> @@ -48,7 +53,8 @@
>>   
>>   #define DWDST			BIT(1)
>>   
>> -static int heartbeat;
>> +static int heartbeat =		DEFAULT_HEARTBEAT;
>> +static u32 wsize =		DEFAULT_WINDOWSIZE;
>>   
>>   /*
>>    * struct to hold data for each WDT device
>> @@ -62,34 +68,93 @@ struct rti_wdt_device {
>>   	struct watchdog_device	wdd;
>>   };
>>   
>> +static int rti_wdt_convert_wsize(void)
>> +{
>> +	if (wsize >= 50) {
>> +		wsize = RTIWWDSIZE_50P;
>> +	} else if (wsize >= 25) {
>> +		wsize = RTIWWDSIZE_25P;
>> +	} else if (wsize > 12) {
>> +		wsize = RTIWWDSIZE_12P5;
>> +	} else if (wsize > 6) {
>> +		wsize = RTIWWDSIZE_6P25;
>> +	} else if (wsize > 3) {
>> +		wsize = RTIWWDSIZE_3P125;
>> +	} else {
>> +		pr_err("%s: bad windowsize: %d\n", __func__, wsize);
> 
> Please do not use pr_ functions. Pass the watchdog device as argument
> and use dev_err().
> 
> Also, this function modifies the wsize parameter. When called
> again, that parameter will have a  totally different meaning, and
> the second call to this function will always set the window size
> to 50.
> 
> On top of all that, window sizes larger than 50 are set to 50,
> window sizes between 4 and 49 are adjusted, and window sizes <= 3
> are rejected. That is not exactly consistent.
> 
> Does this module parameter really add value / make sense ?
> What is the use case ? We should not add such complexity without
> use case.

I'll ditch the support for this. Just thought that it would be neat 
feature to have this in place because I ended up implementing most of 
this for the attach feature anyways.

> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd)
>> +{
>> +	/*
>> +	 * 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:
>> +		pr_err("%s: Bad watchdog window size!\n", __func__);
> 
> Same here.
> 
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int rti_wdt_start(struct watchdog_device *wdd)
>>   {
>>   	u32 timer_margin;
>>   	struct rti_wdt_device *wdt = watchdog_get_drvdata(wdd);
>> +	int ret;
>>   
>>   	/* set timeout period */
>> -	timer_margin = (u64)wdd->timeout * wdt->freq;
>> +	timer_margin = (u64)heartbeat * wdt->freq;
>>   	timer_margin >>= WDT_PRELOAD_SHIFT;
>>   	if (timer_margin > WDT_PRELOAD_MAX)
>>   		timer_margin = WDT_PRELOAD_MAX;
>>   	writel_relaxed(timer_margin, wdt->base + RTIDWDPRLD);
>>   
>> -	/*
>> -	 * 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; 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.
>> -	 */
>> -	wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20;
>> +	ret = rti_wdt_convert_wsize();
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = rti_wdt_setup_hw_hb(wdd);
>> +	if (ret)
>> +		return ret;
>>   
> 
> This is the wrong place to validate the window size. It should be done
> only once, in the probe function. The start function should not fail
> because of a bad window size.
> 
> With such parameters, the wsize written into the chip should be kept
> in struct rti_wdt_device if it needs to be set more than once.
> The module parameter should not be changed, and it should not be used
> to store the register value. min_hw_heartbeat_ms needs to be set in the
> probe function, not in the start function. Sorry that I didn't notice
> that before.

Yeah, this will be gone.

> 
>>   	/* Generate NMI when wdt expires */
>>   	writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL);
>>   
>> -	/* Open window size 50%; this is the largest window size available */
>> -	writel_relaxed(RTIWWDSIZE_50P, wdt->base + RTIWWDSIZECTRL);
>> +	writel_relaxed(wsize, wdt->base + RTIWWDSIZECTRL);
>>   
>>   	readl_relaxed(wdt->base + RTIWWDSIZECTRL);
>>   
>> @@ -169,6 +234,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 > 30000 && wdt->freq < 32768)
>> +		wdt->freq = 30000;
>> +
> 
> Combining that with a window size of 96.9% min heartbeat is asking
> for trouble. It will be all but impossible to catch the window with
> such constraints if the frequency is really that inaccurate.

Yeah, I don't think anything else than 50% window size makes any sense. 
Anyways, will drop the support for configuring this runtime.

> 
>>   	pm_runtime_enable(dev);
>>   	ret = pm_runtime_get_sync(dev);
>>   	if (ret) {
>> @@ -251,5 +324,10 @@ MODULE_PARM_DESC(heartbeat,
>>   		 __MODULE_STRING(MAX_HEARTBEAT) ", default "
>>   		 __MODULE_STRING(DEFAULT_HEARTBEAT));
>>   
>> +module_param(wsize, uint, 0);
>> +MODULE_PARM_DESC(wsize,
>> +		 "Watchdog open window size in percentage from 3 to 50, "
>> +		 "default " __MODULE_STRING(DEFAULT_WINDOW_SIZE));
>> +
>>   MODULE_LICENSE("GPL");
>>   MODULE_ALIAS("platform:rti-wdt");
>>
> 

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

[PATCHv2 4/5] 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 | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index 110bfc8d0bb3..987e5a798cb4 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -213,6 +213,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)
@@ -258,11 +259,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);
@@ -274,12 +272,34 @@ static int rti_wdt_probe(struct platform_device *pdev)
 		goto err_iomap;
 	}
 
+	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
+		u32 time_left;
+
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+		time_left = rti_wdt_get_timeleft(wdd);
+		heartbeat = readl(wdt->base + RTIDWDPRLD);
+		heartbeat <<= WDT_PRELOAD_SHIFT;
+		heartbeat /= wdt->freq;
+
+		wsize = readl(wdt->base + RTIWWDSIZECTRL);
+		ret = rti_wdt_setup_hw_hb(wdd);
+		if (ret)
+			goto err_iomap;
+
+		last_ping = -(time_left - heartbeat) * 1000;
+	}
+
+	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

Re: [PATCHv2 4/5] watchdog: rti-wdt: attach to running watchdog during probe

[Guenter Roeck]

On 7/3/20 5:04 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>
> ---
>  drivers/watchdog/rti_wdt.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index 110bfc8d0bb3..987e5a798cb4 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -213,6 +213,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)
> @@ -258,11 +259,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;

What if the watchdog is not running ?

>  	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);
> @@ -274,12 +272,34 @@ static int rti_wdt_probe(struct platform_device *pdev)
>  		goto err_iomap;
>  	}
>  
> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
> +		u32 time_left;
> +
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +		time_left = rti_wdt_get_timeleft(wdd);
> +		heartbeat = readl(wdt->base + RTIDWDPRLD);
> +		heartbeat <<= WDT_PRELOAD_SHIFT;
> +		heartbeat /= wdt->freq;
> +

This ignores any heartbeat configured as module parameter, which most
people will consider unexpected. It might be worthwhile documenting that.

> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);
> +		ret = rti_wdt_setup_hw_hb(wdd);
> +		if (ret)
> +			goto err_iomap;
> +
> +		last_ping = -(time_left - heartbeat) * 1000;

Why the double negation ?

		last_ping = (heartbeat - time_left) * 1000;

seems simpler. Also, what if heartbeat - time_left is negative for whatever
reason ?

I am not sure if it is a good idea to call rti_wdt_get_timeleft()
here. It might be better to add a helper function such as
rti_wdt_get_timeleft_ms() to return the time left in milli-seconds
for improved accuracy.

> +	}
> +
> +	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: [PATCHv2 4/5] watchdog: rti-wdt: attach to running watchdog during probe

[Tero Kristo]

On 05/07/2020 18:07, Guenter Roeck wrote:
> On 7/3/20 5:04 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>
>> ---
>>   drivers/watchdog/rti_wdt.c | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> index 110bfc8d0bb3..987e5a798cb4 100644
>> --- a/drivers/watchdog/rti_wdt.c
>> +++ b/drivers/watchdog/rti_wdt.c
>> @@ -213,6 +213,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)
>> @@ -258,11 +259,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;
> 
> What if the watchdog is not running ?

Configuring wdd->timeout seems redundant, it gets set by the 
watchdog_init_timeout call done later. I just moved that post the check 
for a running watchdog so that the same call is used for both cases.

> 
>>   	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);
>> @@ -274,12 +272,34 @@ static int rti_wdt_probe(struct platform_device *pdev)
>>   		goto err_iomap;
>>   	}
>>   
>> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
>> +		u32 time_left;
>> +
>> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
>> +		time_left = rti_wdt_get_timeleft(wdd);
>> +		heartbeat = readl(wdt->base + RTIDWDPRLD);
>> +		heartbeat <<= WDT_PRELOAD_SHIFT;
>> +		heartbeat /= wdt->freq;
>> +
> 
> This ignores any heartbeat configured as module parameter, which most
> people will consider unexpected. It might be worthwhile documenting that.

I'll add a dev_warn for this case.

> 
>> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);
>> +		ret = rti_wdt_setup_hw_hb(wdd);
>> +		if (ret)
>> +			goto err_iomap;
>> +
>> +		last_ping = -(time_left - heartbeat) * 1000;
> 
> Why the double negation ?
> 
> 		last_ping = (heartbeat - time_left) * 1000;
> 
> seems simpler. Also, what if heartbeat - time_left is negative for whatever
> reason ?

Will fix. I'll add a dev_warn for that case and assume last ping to be zero.

> 
> I am not sure if it is a good idea to call rti_wdt_get_timeleft()
> here. It might be better to add a helper function such as
> rti_wdt_get_timeleft_ms() to return the time left in milli-seconds
> for improved accuracy.

Will add that.

-Tero

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

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

[PATCHv2 5/5] 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>
---
 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 987e5a798cb4..7007445da80b 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -304,6 +304,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;
 }
@@ -314,6 +315,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: [PATCHv2 5/5] watchdog: rti-wdt: balance pm runtime enable calls

[Guenter Roeck]

On 7/3/20 5:04 AM, Tero Kristo wrote:
> 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 987e5a798cb4..7007445da80b 100644
> --- a/drivers/watchdog/rti_wdt.c
> +++ b/drivers/watchdog/rti_wdt.c
> @@ -304,6 +304,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;
>  }
> @@ -314,6 +315,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;
>  }
>