[PATCHv3 0/4] watchdog: rti: support attach to running timer

[PATCHv3 0/4] watchdog: rti: support attach to running timer

[Tero Kristo]

Hi,

Changes from previous version:

- Documentation changes for patch #2
- Dropped the configurable module parameter for window size
- Merged any needed functionality from old patches #3 and #4 to patch #3
  now
- Added new rti driver internal API for getting remaining milliseconds
  left on the timer

-Tero


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

[PATCHv3 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

[PATCHv3 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

[PATCHv3 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 | 111 +++++++++++++++++++++++++++++++++----
 1 file changed, 101 insertions(+), 10 deletions(-)

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index d456dd72d99a..45dfc546e371 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,47 @@ 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;
+		heartbeat_ms /= wdt->freq;
+		if (heartbeat_ms / 1000 != heartbeat)
+			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
+
+		heartbeat = heartbeat_ms / 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

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

[kernel test robot]

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

Hi Tero,

I love your patch! Yet something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Tero-Kristo/watchdog-rti-support-attach-to-running-timer/20200713-213531
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

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

All errors (new ones prefixed by >>):

   arch/mips/kernel/head.o: in function `dtb_found':
   (.ref.text+0xc8): relocation truncated to fit: R_MIPS_26 against `start_kernel'
   init/main.o: in function `set_reset_devices':
   main.c:(.init.text+0x20): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x30): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `debug_kernel':
   main.c:(.init.text+0x9c): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0xac): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `quiet_kernel':
   main.c:(.init.text+0x118): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x128): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   init/main.o: in function `init_setup':
   main.c:(.init.text+0x1a4): relocation truncated to fit: R_MIPS_26 against `_mcount'
   main.c:(.init.text+0x1c8): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   main.c:(.init.text+0x1e8): relocation truncated to fit: R_MIPS_26 against `__sanitizer_cov_trace_pc'
   main.c:(.init.text+0x1fc): additional relocation overflows omitted from the output
   mips-linux-ld: drivers/watchdog/rti_wdt.o: in function `rti_wdt_probe':
>> rti_wdt.c:(.text.rti_wdt_probe+0x33c): undefined reference to `__udivdi3'
>> mips-linux-ld: rti_wdt.c:(.text.rti_wdt_probe+0x35c): undefined reference to `__udivdi3'

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

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

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

[kernel test robot]

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

Hi Tero,

I love your patch! Yet something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Tero-Kristo/watchdog-rti-support-attach-to-running-timer/20200713-213531
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips 

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

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "__divdi3" [drivers/watchdog/rti_wdt.ko] undefined!
>> ERROR: modpost: "__udivdi3" [drivers/watchdog/rti_wdt.ko] undefined!

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

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

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

[Guenter Roeck]

On 7/13/20 6:18 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 | 111 +++++++++++++++++++++++++++++++++----
>  1 file changed, 101 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
> index d456dd72d99a..45dfc546e371 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;
> +

So this is now only a problem is the window size was set restrictively
in the BOS/ROMMON. Meaning it is still a problem. How is this better than
before ? Why not just always set the window size to something reasonable ?


>  	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,47 @@ 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;
> +		heartbeat_ms /= wdt->freq;

Ah yes, the pitfalls of 64-bit divide operations.

> +		if (heartbeat_ms / 1000 != heartbeat)
> +			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
> +
> +		heartbeat = heartbeat_ms / 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;
> +		}

All that complexity makes me wonder if it wouldn't be better to just reprogram
the watchdog if it is already running. Any reason for not doing that ?

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

[Tero Kristo]

On 14/07/2020 08:15, Guenter Roeck wrote:
> On 7/13/20 6:18 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 | 111 +++++++++++++++++++++++++++++++++----
>>   1 file changed, 101 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>> index d456dd72d99a..45dfc546e371 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;
>> +
> 
> So this is now only a problem is the window size was set restrictively
> in the BOS/ROMMON. Meaning it is still a problem. How is this better than
> before ? Why not just always set the window size to something reasonable ?

Re-programming of the watchdog only takes effect at the next ping, then 
and only then will it adjust the window size + timeout period.

> 
> 
>>   	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,47 @@ 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;
>> +		heartbeat_ms /= wdt->freq;
> 
> Ah yes, the pitfalls of 64-bit divide operations.

Should I convert this to use do_div? With 64bit archs this code is 
targeted at it runs just fine.

> 
>> +		if (heartbeat_ms / 1000 != heartbeat)
>> +			dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
>> +
>> +		heartbeat = heartbeat_ms / 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;
>> +		}
> 
> All that complexity makes me wonder if it wouldn't be better to just reprogram
> the watchdog if it is already running. Any reason for not doing that ?

Well, you can re-program it but... It does not take effect until next 
window starts, so basically we need to take care of the currently 
running window anyways after which re-programming it doesn't make much 
sense anymore. And handling the switch from one setup to next would add 
extra complexity.

-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

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

[Guenter Roeck]

On 7/13/20 11:50 PM, Tero Kristo wrote:
> On 14/07/2020 08:15, Guenter Roeck wrote:
>> On 7/13/20 6:18 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 | 111 +++++++++++++++++++++++++++++++++----
>>>   1 file changed, 101 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>>> index d456dd72d99a..45dfc546e371 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;
>>> +
>>
>> So this is now only a problem is the window size was set restrictively
>> in the BOS/ROMMON. Meaning it is still a problem. How is this better than
>> before ? Why not just always set the window size to something reasonable ?
> 
> Re-programming of the watchdog only takes effect at the next ping, then and only then will it adjust the window size + timeout period.
> 

What a mess. I am glad this isn't hardware I have to deal with.

>>
>>
>>>       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,47 @@ 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;
>>> +        heartbeat_ms /= wdt->freq;
>>
>> Ah yes, the pitfalls of 64-bit divide operations.
> 
> Should I convert this to use do_div? With 64bit archs this code is targeted at it runs just fine.
> 

... and test builds on 32 bit architectures fail to compile, as reported
by 0-day. Maybe you don't care, fine, but then please remove all use of do_div
or other 64-bit divide functions from the driver and mark it as depending
on 64 bit.

>>
>>> +        if (heartbeat_ms / 1000 != heartbeat)
>>> +            dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
>>> +
>>> +        heartbeat = heartbeat_ms / 1000;
>>> +
>>> +        wsize = readl(wdt->base + RTIWWDSIZECTRL);
>>> +        ret = rti_wdt_setup_hw_hb(wdd, wsize);
>>> +        if (ret) {
>>> +            dev_err(dev, "bad window size.\n");

Maybe report what that bad window size actually is ?

>>> +            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;
>>> +        }
>>
>> All that complexity makes me wonder if it wouldn't be better to just reprogram
>> the watchdog if it is already running. Any reason for not doing that ?
> 
> Well, you can re-program it but... It does not take effect until next window starts, so basically we need to take care of the currently running window anyways after which re-programming it doesn't make much sense anymore. And handling the switch from one setup to next would add extra complexity.
> 

Seems to me that hardware team really made an effort to make the
watchdog as difficult to program as possible :-(.

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

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

[Tero Kristo]

On 14/07/2020 10:06, Guenter Roeck wrote:
> On 7/13/20 11:50 PM, Tero Kristo wrote:
>> On 14/07/2020 08:15, Guenter Roeck wrote:
>>> On 7/13/20 6:18 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 | 111 +++++++++++++++++++++++++++++++++----
>>>>    1 file changed, 101 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
>>>> index d456dd72d99a..45dfc546e371 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;
>>>> +
>>>
>>> So this is now only a problem is the window size was set restrictively
>>> in the BOS/ROMMON. Meaning it is still a problem. How is this better than
>>> before ? Why not just always set the window size to something reasonable ?
>>
>> Re-programming of the watchdog only takes effect at the next ping, then and only then will it adjust the window size + timeout period.
>>
> 
> What a mess. I am glad this isn't hardware I have to deal with.

Hahah yeah. :)

> 
>>>
>>>
>>>>        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,47 @@ 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;
>>>> +        heartbeat_ms /= wdt->freq;
>>>
>>> Ah yes, the pitfalls of 64-bit divide operations.
>>
>> Should I convert this to use do_div? With 64bit archs this code is targeted at it runs just fine.
>>
> 
> ... and test builds on 32 bit architectures fail to compile, as reported
> by 0-day. Maybe you don't care, fine, but then please remove all use of do_div
> or other 64-bit divide functions from the driver and mark it as depending
> on 64 bit.

Let me try to fix this to compile on 32bit archs also. It probably is 
never going to be run on such setup, but should be possible to fix it.

> 
>>>
>>>> +        if (heartbeat_ms / 1000 != heartbeat)
>>>> +            dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n");
>>>> +
>>>> +        heartbeat = heartbeat_ms / 1000;
>>>> +
>>>> +        wsize = readl(wdt->base + RTIWWDSIZECTRL);
>>>> +        ret = rti_wdt_setup_hw_hb(wdd, wsize);
>>>> +        if (ret) {
>>>> +            dev_err(dev, "bad window size.\n");
> 
> Maybe report what that bad window size actually is ?

Ok, will print out the register value here.

> 
>>>> +            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;
>>>> +        }
>>>
>>> All that complexity makes me wonder if it wouldn't be better to just reprogram
>>> the watchdog if it is already running. Any reason for not doing that ?
>>
>> Well, you can re-program it but... It does not take effect until next window starts, so basically we need to take care of the currently running window anyways after which re-programming it doesn't make much sense anymore. And handling the switch from one setup to next would add extra complexity.
>>
> 
> Seems to me that hardware team really made an effort to make the
> watchdog as difficult to program as possible :-(.

Yea, it is surprisingly difficult to program... when watchdogs in 
principle are extremely simple pieces of HW. This claims to be some 
automotive grade watchdog, which means it has the window in place.

-Tero

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

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

[Guenter Roeck]

On 7/14/20 12:13 AM, Tero Kristo wrote:
[ ... ]

>>> Well, you can re-program it but... It does not take effect until next window starts, so basically we need to take care of the currently running window anyways after which re-programming it doesn't make much sense anymore. And handling the switch from one setup to next would add extra complexity.
>>>
>>
>> Seems to me that hardware team really made an effort to make the
>> watchdog as difficult to program as possible :-(.
> 
> Yea, it is surprisingly difficult to program... when watchdogs in principle are extremely simple pieces of HW. This claims to be some automotive grade watchdog, which means it has the window in place.
> 

We are getting a bit off topic, but that almost sounds like
"automotive" translates to "it must be hard to program".
I can not think of a valid reason for such a window, no matter
the circumstances. Either case, dealing with the window in the
kernel in that situation (ie if some specification states that
the window must exist) would be the wrong solution and circumvent
the reason for having it in the first place.

Not that this matters here, of course. I am just wondering what the impact
would be if the driver is indeed used in such an application. For example,
some compliance test code might try to ping the watchdog outside the window
to test its reaction. Such test code would fail if the ping is delayed
by the kernel.

Guenter

[PATCHv3 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 45dfc546e371..5cc1a1d654b6 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -302,6 +302,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;
 }
@@ -312,6 +313,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