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 <[email protected]>
---
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
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 <[email protected]>
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/[email protected]
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 <[email protected]>
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/[email protected]
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 <[email protected]>
> ---
> 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:
>
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>>>> ---
>>>> 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
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