* [PATCHv4 0/4] watchdog: rti-wdt: attach to running timer
@ 2020-07-17 13:29 Tero Kristo
2020-07-17 13:29 ` [PATCHv4 1/4] watchdog: use __watchdog_ping in startup Tero Kristo
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Tero Kristo @ 2020-07-17 13:29 UTC (permalink / raw)
To: wim, linux, linux-watchdog; +Cc: linux-kernel, jan.kiszka
Hi,
V4 of this series only fixes 32bit compile issue with patch #3. Appears
to be compiling now fine with ARM32 allmodconfig.
-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCHv4 1/4] watchdog: use __watchdog_ping in startup 2020-07-17 13:29 [PATCHv4 0/4] watchdog: rti-wdt: attach to running timer Tero Kristo @ 2020-07-17 13:29 ` Tero Kristo 2020-07-17 13:29 ` [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time Tero Kristo ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: Tero Kristo @ 2020-07-17 13:29 UTC (permalink / raw) To: wim, linux, linux-watchdog; +Cc: linux-kernel, jan.kiszka 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time 2020-07-17 13:29 [PATCHv4 0/4] watchdog: rti-wdt: attach to running timer Tero Kristo 2020-07-17 13:29 ` [PATCHv4 1/4] watchdog: use __watchdog_ping in startup Tero Kristo @ 2020-07-17 13:29 ` Tero Kristo 2020-07-19 13:54 ` Guenter Roeck 2020-07-17 13:29 ` [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe Tero Kristo 2020-07-17 13:29 ` [PATCHv4 4/4] watchdog: rti-wdt: balance pm runtime enable calls Tero Kristo 3 siblings, 1 reply; 13+ messages in thread From: Tero Kristo @ 2020-07-17 13:29 UTC (permalink / raw) To: wim, linux, linux-watchdog; +Cc: linux-kernel, jan.kiszka 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time 2020-07-17 13:29 ` [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time Tero Kristo @ 2020-07-19 13:54 ` Guenter Roeck 0 siblings, 0 replies; 13+ messages in thread From: Guenter Roeck @ 2020-07-19 13:54 UTC (permalink / raw) To: Tero Kristo, wim, linux-watchdog; +Cc: linux-kernel, jan.kiszka On 7/17/20 6:29 AM, Tero Kristo wrote: > Certain watchdogs require the watchdog only to be pinged within a > specific time window, pinging too early or too late cause the watchdog > to fire. In cases where this sort of watchdog has been started before > kernel comes up, we must adjust the watchdog keepalive window to match > the actually running timer, so add a new driver API for this purpose. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > .../watchdog/watchdog-kernel-api.rst | 12 ++++++++ > drivers/watchdog/watchdog_dev.c | 30 +++++++++++++++++++ > include/linux/watchdog.h | 2 ++ > 3 files changed, 44 insertions(+) > > diff --git a/Documentation/watchdog/watchdog-kernel-api.rst b/Documentation/watchdog/watchdog-kernel-api.rst > index 068a55ee0d4a..baf44e986b07 100644 > --- a/Documentation/watchdog/watchdog-kernel-api.rst > +++ b/Documentation/watchdog/watchdog-kernel-api.rst > @@ -336,3 +336,15 @@ an action is taken by a preconfigured pretimeout governor preassigned to > the watchdog device. If watchdog pretimeout governor framework is not > enabled, watchdog_notify_pretimeout() prints a notification message to > the kernel log buffer. > + > +To set the last known HW keepalive time for a watchdog, the following function > +should be used:: > + > + int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd, > + unsigned int last_ping_ms) > + > +This function must be called immediately after watchdog registration. It > +sets the last known hardware heartbeat to have happened last_ping_ms before > +current time. Calling this is only needed if the watchdog is already running > +when probe is called, and the watchdog can only be pinged after the > +min_hw_heartbeat_ms time has passed from the last ping. > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index bc1cfa288553..e74a0c6811b5 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -1138,6 +1138,36 @@ void watchdog_dev_unregister(struct watchdog_device *wdd) > watchdog_cdev_unregister(wdd); > } > > +/* > + * watchdog_set_last_hw_keepalive: set last HW keepalive time for watchdog > + * @wdd: watchdog device > + * @last_ping_ms: time since last HW heartbeat > + * > + * Adjusts the last known HW keepalive time for a watchdog timer. > + * This is needed if the watchdog is already running when the probe > + * function is called, and it can't be pinged immediately. This > + * function must be called immediately after watchdog registration, > + * and min_hw_heartbeat_ms must be set for this to be useful. > + */ > +int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd, > + unsigned int last_ping_ms) > +{ > + struct watchdog_core_data *wd_data; > + ktime_t now; > + > + if (!wdd) > + return -EINVAL; > + > + wd_data = wdd->wd_data; > + > + now = ktime_get(); > + > + wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms)); > + > + return __watchdog_ping(wdd); > +} > +EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive); > + > /* > * watchdog_dev_init: init dev part of watchdog core > * > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h > index 1464ce6ffa31..9b19e6bb68b5 100644 > --- a/include/linux/watchdog.h > +++ b/include/linux/watchdog.h > @@ -210,6 +210,8 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd, > extern int watchdog_register_device(struct watchdog_device *); > extern void watchdog_unregister_device(struct watchdog_device *); > > +int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int); > + > /* devres register variant */ > int devm_watchdog_register_device(struct device *dev, struct watchdog_device *); > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe 2020-07-17 13:29 [PATCHv4 0/4] watchdog: rti-wdt: attach to running timer Tero Kristo 2020-07-17 13:29 ` [PATCHv4 1/4] watchdog: use __watchdog_ping in startup Tero Kristo 2020-07-17 13:29 ` [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time Tero Kristo @ 2020-07-17 13:29 ` Tero Kristo 2020-07-19 13:56 ` Guenter Roeck 2022-02-21 9:10 ` Jan Kiszka 2020-07-17 13:29 ` [PATCHv4 4/4] watchdog: rti-wdt: balance pm runtime enable calls Tero Kristo 3 siblings, 2 replies; 13+ messages in thread From: Tero Kristo @ 2020-07-17 13:29 UTC (permalink / raw) To: wim, linux, linux-watchdog; +Cc: linux-kernel, jan.kiszka If the RTI watchdog is running already during probe, the driver must configure itself to match the HW. Window size and timeout is probed from hardware, and the last keepalive ping is adjusted to match it also. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++---- 1 file changed, 102 insertions(+), 10 deletions(-) diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c index d456dd72d99a..7cbdc178ffe8 100644 --- a/drivers/watchdog/rti_wdt.c +++ b/drivers/watchdog/rti_wdt.c @@ -35,7 +35,11 @@ #define RTIWWDRX_NMI 0xa -#define RTIWWDSIZE_50P 0x50 +#define RTIWWDSIZE_50P 0x50 +#define RTIWWDSIZE_25P 0x500 +#define RTIWWDSIZE_12P5 0x5000 +#define RTIWWDSIZE_6P25 0x50000 +#define RTIWWDSIZE_3P125 0x500000 #define WDENABLE_KEY 0xa98559da @@ -48,7 +52,7 @@ #define DWDST BIT(1) -static int heartbeat; +static int heartbeat = DEFAULT_HEARTBEAT; /* * struct to hold data for each WDT device @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd) * be petted during the open window; not too early or not too late. * The HW configuration options only allow for the open window size * to be 50% or less than that; we obviouly want to configure the open - * window as large as possible so we select the 50% option. To avoid - * any glitches, we accommodate 5% safety margin also, so we setup - * the min_hw_hearbeat at 55% of the timeout period. + * window as large as possible so we select the 50% option. */ - wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20; + wdd->min_hw_heartbeat_ms = 500 * wdd->timeout; /* Generate NMI when wdt expires */ writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL); @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd) return 0; } -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize) +{ + /* + * RTI only supports a windowed mode, where the watchdog can only + * be petted during the open window; not too early or not too late. + * The HW configuration options only allow for the open window size + * to be 50% or less than that. + */ + switch (wsize) { + case RTIWWDSIZE_50P: + /* 50% open window => 50% min heartbeat */ + wdd->min_hw_heartbeat_ms = 500 * heartbeat; + break; + + case RTIWWDSIZE_25P: + /* 25% open window => 75% min heartbeat */ + wdd->min_hw_heartbeat_ms = 750 * heartbeat; + break; + + case RTIWWDSIZE_12P5: + /* 12.5% open window => 87.5% min heartbeat */ + wdd->min_hw_heartbeat_ms = 875 * heartbeat; + break; + + case RTIWWDSIZE_6P25: + /* 6.5% open window => 93.5% min heartbeat */ + wdd->min_hw_heartbeat_ms = 935 * heartbeat; + break; + + case RTIWWDSIZE_3P125: + /* 3.125% open window => 96.9% min heartbeat */ + wdd->min_hw_heartbeat_ms = 969 * heartbeat; + break; + + default: + return -EINVAL; + } + + return 0; +} + +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) { u64 timer_counter; u32 val; @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR); + timer_counter *= 1000; + do_div(timer_counter, wdt->freq); return timer_counter; } +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) +{ + return rti_wdt_get_timeleft_ms(wdd) / 1000; +} + static const struct watchdog_info rti_wdt_info = { .options = WDIOF_KEEPALIVEPING, .identity = "K3 RTI Watchdog", @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev) struct watchdog_device *wdd; struct rti_wdt_device *wdt; struct clk *clk; + u32 last_ping = 0; wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); if (!wdt) @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev) return -EINVAL; } + /* + * If watchdog is running at 32k clock, it is not accurate. + * Adjust frequency down in this case so that we don't pet + * the watchdog too often. + */ + if (wdt->freq < 32768) + wdt->freq = wdt->freq * 9 / 10; + pm_runtime_enable(dev); ret = pm_runtime_get_sync(dev); if (ret) { @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev) wdd->min_timeout = 1; wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / wdt->freq * 1000; - wdd->timeout = DEFAULT_HEARTBEAT; wdd->parent = dev; - watchdog_init_timeout(wdd, heartbeat, dev); - watchdog_set_drvdata(wdd, wdt); watchdog_set_nowayout(wdd, 1); watchdog_set_restart_priority(wdd, 128); @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev) goto err_iomap; } + if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { + u32 time_left_ms; + u64 heartbeat_ms; + u32 wsize; + + set_bit(WDOG_HW_RUNNING, &wdd->status); + time_left_ms = rti_wdt_get_timeleft_ms(wdd); + heartbeat_ms = readl(wdt->base + RTIDWDPRLD); + heartbeat_ms <<= WDT_PRELOAD_SHIFT; + heartbeat_ms *= 1000; + do_div(heartbeat_ms, wdt->freq); + if (heartbeat_ms != heartbeat * 1000) + dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); + + heartbeat = heartbeat_ms; + heartbeat /= 1000; + + wsize = readl(wdt->base + RTIWWDSIZECTRL); + ret = rti_wdt_setup_hw_hb(wdd, wsize); + if (ret) { + dev_err(dev, "bad window size.\n"); + goto err_iomap; + } + + last_ping = heartbeat_ms - time_left_ms; + if (time_left_ms > heartbeat_ms) { + dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n"); + last_ping = 0; + } + } + + watchdog_init_timeout(wdd, heartbeat, dev); + ret = watchdog_register_device(wdd); if (ret) { dev_err(dev, "cannot register watchdog device\n"); goto err_iomap; } + if (last_ping) + watchdog_set_last_hw_keepalive(wdd, last_ping); + return 0; err_iomap: -- 2.17.1 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe 2020-07-17 13:29 ` [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe Tero Kristo @ 2020-07-19 13:56 ` Guenter Roeck 2022-02-21 9:10 ` Jan Kiszka 1 sibling, 0 replies; 13+ messages in thread From: Guenter Roeck @ 2020-07-19 13:56 UTC (permalink / raw) To: Tero Kristo, wim, linux-watchdog; +Cc: linux-kernel, jan.kiszka On 7/17/20 6:29 AM, Tero Kristo wrote: > If the RTI watchdog is running already during probe, the driver must > configure itself to match the HW. Window size and timeout is probed from > hardware, and the last keepalive ping is adjusted to match it also. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++---- > 1 file changed, 102 insertions(+), 10 deletions(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index d456dd72d99a..7cbdc178ffe8 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -35,7 +35,11 @@ > > #define RTIWWDRX_NMI 0xa > > -#define RTIWWDSIZE_50P 0x50 > +#define RTIWWDSIZE_50P 0x50 > +#define RTIWWDSIZE_25P 0x500 > +#define RTIWWDSIZE_12P5 0x5000 > +#define RTIWWDSIZE_6P25 0x50000 > +#define RTIWWDSIZE_3P125 0x500000 > > #define WDENABLE_KEY 0xa98559da > > @@ -48,7 +52,7 @@ > > #define DWDST BIT(1) > > -static int heartbeat; > +static int heartbeat = DEFAULT_HEARTBEAT; > > /* > * struct to hold data for each WDT device > @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd) > * be petted during the open window; not too early or not too late. > * The HW configuration options only allow for the open window size > * to be 50% or less than that; we obviouly want to configure the open > - * window as large as possible so we select the 50% option. To avoid > - * any glitches, we accommodate 5% safety margin also, so we setup > - * the min_hw_hearbeat at 55% of the timeout period. > + * window as large as possible so we select the 50% option. > */ > - wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20; > + wdd->min_hw_heartbeat_ms = 500 * wdd->timeout; > > /* Generate NMI when wdt expires */ > writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL); > @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd) > return 0; > } > > -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize) > +{ > + /* > + * RTI only supports a windowed mode, where the watchdog can only > + * be petted during the open window; not too early or not too late. > + * The HW configuration options only allow for the open window size > + * to be 50% or less than that. > + */ > + switch (wsize) { > + case RTIWWDSIZE_50P: > + /* 50% open window => 50% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 500 * heartbeat; > + break; > + > + case RTIWWDSIZE_25P: > + /* 25% open window => 75% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 750 * heartbeat; > + break; > + > + case RTIWWDSIZE_12P5: > + /* 12.5% open window => 87.5% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 875 * heartbeat; > + break; > + > + case RTIWWDSIZE_6P25: > + /* 6.5% open window => 93.5% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 935 * heartbeat; > + break; > + > + case RTIWWDSIZE_3P125: > + /* 3.125% open window => 96.9% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 969 * heartbeat; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) > { > u64 timer_counter; > u32 val; > @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > > timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR); > > + timer_counter *= 1000; > + > do_div(timer_counter, wdt->freq); > > return timer_counter; > } > > +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > +{ > + return rti_wdt_get_timeleft_ms(wdd) / 1000; > +} > + > static const struct watchdog_info rti_wdt_info = { > .options = WDIOF_KEEPALIVEPING, > .identity = "K3 RTI Watchdog", > @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev) > struct watchdog_device *wdd; > struct rti_wdt_device *wdt; > struct clk *clk; > + u32 last_ping = 0; > > wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > if (!wdt) > @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev) > return -EINVAL; > } > > + /* > + * If watchdog is running at 32k clock, it is not accurate. > + * Adjust frequency down in this case so that we don't pet > + * the watchdog too often. > + */ > + if (wdt->freq < 32768) > + wdt->freq = wdt->freq * 9 / 10; > + > pm_runtime_enable(dev); > ret = pm_runtime_get_sync(dev); > if (ret) { > @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev) > wdd->min_timeout = 1; > wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / > wdt->freq * 1000; > - wdd->timeout = DEFAULT_HEARTBEAT; > wdd->parent = dev; > > - watchdog_init_timeout(wdd, heartbeat, dev); > - > watchdog_set_drvdata(wdd, wdt); > watchdog_set_nowayout(wdd, 1); > watchdog_set_restart_priority(wdd, 128); > @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev) > goto err_iomap; > } > > + if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { > + u32 time_left_ms; > + u64 heartbeat_ms; > + u32 wsize; > + > + set_bit(WDOG_HW_RUNNING, &wdd->status); > + time_left_ms = rti_wdt_get_timeleft_ms(wdd); > + heartbeat_ms = readl(wdt->base + RTIDWDPRLD); > + heartbeat_ms <<= WDT_PRELOAD_SHIFT; > + heartbeat_ms *= 1000; > + do_div(heartbeat_ms, wdt->freq); > + if (heartbeat_ms != heartbeat * 1000) > + dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); > + > + heartbeat = heartbeat_ms; > + heartbeat /= 1000; > + > + wsize = readl(wdt->base + RTIWWDSIZECTRL); > + ret = rti_wdt_setup_hw_hb(wdd, wsize); > + if (ret) { > + dev_err(dev, "bad window size.\n"); > + goto err_iomap; > + } > + > + last_ping = heartbeat_ms - time_left_ms; > + if (time_left_ms > heartbeat_ms) { > + dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n"); > + last_ping = 0; > + } > + } > + > + watchdog_init_timeout(wdd, heartbeat, dev); > + > ret = watchdog_register_device(wdd); > if (ret) { > dev_err(dev, "cannot register watchdog device\n"); > goto err_iomap; > } > > + if (last_ping) > + watchdog_set_last_hw_keepalive(wdd, last_ping); > + > return 0; > > err_iomap: > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe 2020-07-17 13:29 ` [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe Tero Kristo 2020-07-19 13:56 ` Guenter Roeck @ 2022-02-21 9:10 ` Jan Kiszka 2022-02-21 9:59 ` Jan Kiszka 1 sibling, 1 reply; 13+ messages in thread From: Jan Kiszka @ 2022-02-21 9:10 UTC (permalink / raw) To: Tero Kristo, wim, linux, linux-watchdog; +Cc: linux-kernel On 17.07.20 15:29, Tero Kristo wrote: > If the RTI watchdog is running already during probe, the driver must > configure itself to match the HW. Window size and timeout is probed from > hardware, and the last keepalive ping is adjusted to match it also. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++---- > 1 file changed, 102 insertions(+), 10 deletions(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index d456dd72d99a..7cbdc178ffe8 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -35,7 +35,11 @@ > > #define RTIWWDRX_NMI 0xa > > -#define RTIWWDSIZE_50P 0x50 > +#define RTIWWDSIZE_50P 0x50 > +#define RTIWWDSIZE_25P 0x500 > +#define RTIWWDSIZE_12P5 0x5000 > +#define RTIWWDSIZE_6P25 0x50000 > +#define RTIWWDSIZE_3P125 0x500000 > > #define WDENABLE_KEY 0xa98559da > > @@ -48,7 +52,7 @@ > > #define DWDST BIT(1) > > -static int heartbeat; > +static int heartbeat = DEFAULT_HEARTBEAT; > > /* > * struct to hold data for each WDT device > @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd) > * be petted during the open window; not too early or not too late. > * The HW configuration options only allow for the open window size > * to be 50% or less than that; we obviouly want to configure the open > - * window as large as possible so we select the 50% option. To avoid > - * any glitches, we accommodate 5% safety margin also, so we setup > - * the min_hw_hearbeat at 55% of the timeout period. > + * window as large as possible so we select the 50% option. > */ > - wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20; > + wdd->min_hw_heartbeat_ms = 500 * wdd->timeout; > > /* Generate NMI when wdt expires */ > writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL); > @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd) > return 0; > } > > -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize) > +{ > + /* > + * RTI only supports a windowed mode, where the watchdog can only > + * be petted during the open window; not too early or not too late. > + * The HW configuration options only allow for the open window size > + * to be 50% or less than that. > + */ > + switch (wsize) { > + case RTIWWDSIZE_50P: > + /* 50% open window => 50% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 500 * heartbeat; > + break; > + > + case RTIWWDSIZE_25P: > + /* 25% open window => 75% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 750 * heartbeat; > + break; > + > + case RTIWWDSIZE_12P5: > + /* 12.5% open window => 87.5% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 875 * heartbeat; > + break; > + > + case RTIWWDSIZE_6P25: > + /* 6.5% open window => 93.5% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 935 * heartbeat; > + break; > + > + case RTIWWDSIZE_3P125: > + /* 3.125% open window => 96.9% min heartbeat */ > + wdd->min_hw_heartbeat_ms = 969 * heartbeat; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) > { > u64 timer_counter; > u32 val; > @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > > timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR); > > + timer_counter *= 1000; > + > do_div(timer_counter, wdt->freq); > > return timer_counter; > } > > +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > +{ > + return rti_wdt_get_timeleft_ms(wdd) / 1000; > +} > + > static const struct watchdog_info rti_wdt_info = { > .options = WDIOF_KEEPALIVEPING, > .identity = "K3 RTI Watchdog", > @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev) > struct watchdog_device *wdd; > struct rti_wdt_device *wdt; > struct clk *clk; > + u32 last_ping = 0; > > wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > if (!wdt) > @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev) > return -EINVAL; > } > > + /* > + * If watchdog is running at 32k clock, it is not accurate. > + * Adjust frequency down in this case so that we don't pet > + * the watchdog too often. > + */ > + if (wdt->freq < 32768) > + wdt->freq = wdt->freq * 9 / 10; > + This seems broken: You are only adjusting the frequency value used by the driver. What has been programmed into the hardware already is still based on real frequency. Moreover, this path is not only taken when the watchdog is already running - but the latter case is what the subject and commit message suggests. I've noticed this by comparing bootloader-programmed values with those the driver assumes to see - 10% off, obviously. So, what is actually supposed to happen here? Jan > pm_runtime_enable(dev); > ret = pm_runtime_get_sync(dev); > if (ret) { > @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev) > wdd->min_timeout = 1; > wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / > wdt->freq * 1000; > - wdd->timeout = DEFAULT_HEARTBEAT; > wdd->parent = dev; > > - watchdog_init_timeout(wdd, heartbeat, dev); > - > watchdog_set_drvdata(wdd, wdt); > watchdog_set_nowayout(wdd, 1); > watchdog_set_restart_priority(wdd, 128); > @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev) > goto err_iomap; > } > > + if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { > + u32 time_left_ms; > + u64 heartbeat_ms; > + u32 wsize; > + > + set_bit(WDOG_HW_RUNNING, &wdd->status); > + time_left_ms = rti_wdt_get_timeleft_ms(wdd); > + heartbeat_ms = readl(wdt->base + RTIDWDPRLD); > + heartbeat_ms <<= WDT_PRELOAD_SHIFT; > + heartbeat_ms *= 1000; > + do_div(heartbeat_ms, wdt->freq); > + if (heartbeat_ms != heartbeat * 1000) > + dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); > + > + heartbeat = heartbeat_ms; > + heartbeat /= 1000; > + > + wsize = readl(wdt->base + RTIWWDSIZECTRL); > + ret = rti_wdt_setup_hw_hb(wdd, wsize); > + if (ret) { > + dev_err(dev, "bad window size.\n"); > + goto err_iomap; > + } > + > + last_ping = heartbeat_ms - time_left_ms; > + if (time_left_ms > heartbeat_ms) { > + dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n"); > + last_ping = 0; > + } > + } > + > + watchdog_init_timeout(wdd, heartbeat, dev); > + > ret = watchdog_register_device(wdd); > if (ret) { > dev_err(dev, "cannot register watchdog device\n"); > goto err_iomap; > } > > + if (last_ping) > + watchdog_set_last_hw_keepalive(wdd, last_ping); > + > return 0; > > err_iomap: -- Siemens AG, Technology Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe 2022-02-21 9:10 ` Jan Kiszka @ 2022-02-21 9:59 ` Jan Kiszka 2022-02-21 12:44 ` Nishanth Menon 0 siblings, 1 reply; 13+ messages in thread From: Jan Kiszka @ 2022-02-21 9:59 UTC (permalink / raw) To: wim, linux, linux-watchdog, Nishanth Menon; +Cc: linux-kernel On 21.02.22 10:10, Jan Kiszka wrote: > On 17.07.20 15:29, Tero Kristo wrote: >> If the RTI watchdog is running already during probe, the driver must >> configure itself to match the HW. Window size and timeout is probed from >> hardware, and the last keepalive ping is adjusted to match it also. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++---- >> 1 file changed, 102 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c >> index d456dd72d99a..7cbdc178ffe8 100644 >> --- a/drivers/watchdog/rti_wdt.c >> +++ b/drivers/watchdog/rti_wdt.c >> @@ -35,7 +35,11 @@ >> >> #define RTIWWDRX_NMI 0xa >> >> -#define RTIWWDSIZE_50P 0x50 >> +#define RTIWWDSIZE_50P 0x50 >> +#define RTIWWDSIZE_25P 0x500 >> +#define RTIWWDSIZE_12P5 0x5000 >> +#define RTIWWDSIZE_6P25 0x50000 >> +#define RTIWWDSIZE_3P125 0x500000 >> >> #define WDENABLE_KEY 0xa98559da >> >> @@ -48,7 +52,7 @@ >> >> #define DWDST BIT(1) >> >> -static int heartbeat; >> +static int heartbeat = DEFAULT_HEARTBEAT; >> >> /* >> * struct to hold data for each WDT device >> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd) >> * be petted during the open window; not too early or not too late. >> * The HW configuration options only allow for the open window size >> * to be 50% or less than that; we obviouly want to configure the open >> - * window as large as possible so we select the 50% option. To avoid >> - * any glitches, we accommodate 5% safety margin also, so we setup >> - * the min_hw_hearbeat at 55% of the timeout period. >> + * window as large as possible so we select the 50% option. >> */ >> - wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20; >> + wdd->min_hw_heartbeat_ms = 500 * wdd->timeout; >> >> /* Generate NMI when wdt expires */ >> writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL); >> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd) >> return 0; >> } >> >> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize) >> +{ >> + /* >> + * RTI only supports a windowed mode, where the watchdog can only >> + * be petted during the open window; not too early or not too late. >> + * The HW configuration options only allow for the open window size >> + * to be 50% or less than that. >> + */ >> + switch (wsize) { >> + case RTIWWDSIZE_50P: >> + /* 50% open window => 50% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 500 * heartbeat; >> + break; >> + >> + case RTIWWDSIZE_25P: >> + /* 25% open window => 75% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 750 * heartbeat; >> + break; >> + >> + case RTIWWDSIZE_12P5: >> + /* 12.5% open window => 87.5% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 875 * heartbeat; >> + break; >> + >> + case RTIWWDSIZE_6P25: >> + /* 6.5% open window => 93.5% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 935 * heartbeat; >> + break; >> + >> + case RTIWWDSIZE_3P125: >> + /* 3.125% open window => 96.9% min heartbeat */ >> + wdd->min_hw_heartbeat_ms = 969 * heartbeat; >> + break; >> + >> + default: >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) >> { >> u64 timer_counter; >> u32 val; >> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >> >> timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR); >> >> + timer_counter *= 1000; >> + >> do_div(timer_counter, wdt->freq); >> >> return timer_counter; >> } >> >> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >> +{ >> + return rti_wdt_get_timeleft_ms(wdd) / 1000; >> +} >> + >> static const struct watchdog_info rti_wdt_info = { >> .options = WDIOF_KEEPALIVEPING, >> .identity = "K3 RTI Watchdog", >> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev) >> struct watchdog_device *wdd; >> struct rti_wdt_device *wdt; >> struct clk *clk; >> + u32 last_ping = 0; >> >> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); >> if (!wdt) >> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev) >> return -EINVAL; >> } >> >> + /* >> + * If watchdog is running at 32k clock, it is not accurate. >> + * Adjust frequency down in this case so that we don't pet >> + * the watchdog too often. >> + */ >> + if (wdt->freq < 32768) >> + wdt->freq = wdt->freq * 9 / 10; >> + > > This seems broken: You are only adjusting the frequency value used by > the driver. What has been programmed into the hardware already is still > based on real frequency. Moreover, this path is not only taken when the > watchdog is already running - but the latter case is what the subject > and commit message suggests. I've noticed this by comparing > bootloader-programmed values with those the driver assumes to see - 10% > off, obviously. > > So, what is actually supposed to happen here? > > Jan > >> pm_runtime_enable(dev); >> ret = pm_runtime_get_sync(dev); >> if (ret) { >> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev) >> wdd->min_timeout = 1; >> wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / >> wdt->freq * 1000; >> - wdd->timeout = DEFAULT_HEARTBEAT; >> wdd->parent = dev; >> >> - watchdog_init_timeout(wdd, heartbeat, dev); >> - >> watchdog_set_drvdata(wdd, wdt); >> watchdog_set_nowayout(wdd, 1); >> watchdog_set_restart_priority(wdd, 128); >> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev) >> goto err_iomap; >> } >> >> + if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { >> + u32 time_left_ms; >> + u64 heartbeat_ms; >> + u32 wsize; >> + >> + set_bit(WDOG_HW_RUNNING, &wdd->status); >> + time_left_ms = rti_wdt_get_timeleft_ms(wdd); >> + heartbeat_ms = readl(wdt->base + RTIDWDPRLD); >> + heartbeat_ms <<= WDT_PRELOAD_SHIFT; >> + heartbeat_ms *= 1000; >> + do_div(heartbeat_ms, wdt->freq); >> + if (heartbeat_ms != heartbeat * 1000) >> + dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); >> + >> + heartbeat = heartbeat_ms; >> + heartbeat /= 1000; >> + >> + wsize = readl(wdt->base + RTIWWDSIZECTRL); >> + ret = rti_wdt_setup_hw_hb(wdd, wsize); >> + if (ret) { >> + dev_err(dev, "bad window size.\n"); >> + goto err_iomap; >> + } >> + >> + last_ping = heartbeat_ms - time_left_ms; >> + if (time_left_ms > heartbeat_ms) { >> + dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n"); >> + last_ping = 0; >> + } >> + } >> + >> + watchdog_init_timeout(wdd, heartbeat, dev); >> + >> ret = watchdog_register_device(wdd); >> if (ret) { >> dev_err(dev, "cannot register watchdog device\n"); >> goto err_iomap; >> } >> >> + if (last_ping) >> + watchdog_set_last_hw_keepalive(wdd, last_ping); >> + >> return 0; >> >> err_iomap: > There is actually more "inaccurate". For now, I would try to address it like this: diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c index 88815419ad1a..1b6629fa5bfc 100644 --- a/drivers/watchdog/rti_wdt.c +++ b/drivers/watchdog/rti_wdt.c @@ -231,14 +231,6 @@ static int rti_wdt_probe(struct platform_device *pdev) return -EINVAL; } - /* - * If watchdog is running at 32k clock, it is not accurate. - * Adjust frequency down in this case so that we don't pet - * the watchdog too often. - */ - if (wdt->freq < 32768) - wdt->freq = wdt->freq * 9 / 10; - pm_runtime_enable(dev); ret = pm_runtime_get_sync(dev); if (ret) { @@ -252,8 +244,6 @@ static int rti_wdt_probe(struct platform_device *pdev) wdd->info = &rti_wdt_info; wdd->ops = &rti_wdt_ops; wdd->min_timeout = 1; - wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / - wdt->freq * 1000; wdd->parent = dev; watchdog_set_drvdata(wdd, wdt); @@ -280,7 +270,7 @@ static int rti_wdt_probe(struct platform_device *pdev) if (heartbeat_ms != heartbeat * 1000) dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); - heartbeat = heartbeat_ms; + heartbeat = heartbeat_ms + 500; heartbeat /= 1000; wsize = readl(wdt->base + RTIWWDSIZECTRL); @@ -297,6 +287,17 @@ static int rti_wdt_probe(struct platform_device *pdev) } } + /* + * If watchdog is running at 32k clock, it is not accurate. + * Adjust frequency down in this case so that we don't pet + * the watchdog too often. + */ + if (wdt->freq < 32768) + wdt->freq = wdt->freq * 9 / 10; + + wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / + wdt->freq * 1000; + watchdog_init_timeout(wdd, heartbeat, dev); ret = watchdog_register_device(wdd); This moves the virtual frequency tweaking after reading back the programmed timeout. It also properly rounds that up to full seconds so that, e.g., bootloader-programmed programmed 60s will become 59s in the driver. That could have led to too short min_hw_heartbeat_ms. I can send this out as real patch, but I'd still like to understand the freq fiddling first. Is that addressing a real hardware issue? Or was it actually papering over that round-up problem? Jan -- Siemens AG, Technology Competence Center Embedded Linux ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe 2022-02-21 9:59 ` Jan Kiszka @ 2022-02-21 12:44 ` Nishanth Menon 2022-02-21 12:53 ` Jan Kiszka 0 siblings, 1 reply; 13+ messages in thread From: Nishanth Menon @ 2022-02-21 12:44 UTC (permalink / raw) To: Jan Kiszka, Hari Nagalla; +Cc: wim, linux, linux-watchdog, linux-kernel On 10:59-20220221, Jan Kiszka wrote: > On 21.02.22 10:10, Jan Kiszka wrote: > > On 17.07.20 15:29, Tero Kristo wrote: > >> If the RTI watchdog is running already during probe, the driver must > >> configure itself to match the HW. Window size and timeout is probed from > >> hardware, and the last keepalive ping is adjusted to match it also. > >> > >> Signed-off-by: Tero Kristo <t-kristo@ti.com> > >> --- > >> drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++---- > >> 1 file changed, 102 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > >> index d456dd72d99a..7cbdc178ffe8 100644 > >> --- a/drivers/watchdog/rti_wdt.c > >> +++ b/drivers/watchdog/rti_wdt.c > >> @@ -35,7 +35,11 @@ > >> > >> #define RTIWWDRX_NMI 0xa > >> > >> -#define RTIWWDSIZE_50P 0x50 > >> +#define RTIWWDSIZE_50P 0x50 > >> +#define RTIWWDSIZE_25P 0x500 > >> +#define RTIWWDSIZE_12P5 0x5000 > >> +#define RTIWWDSIZE_6P25 0x50000 > >> +#define RTIWWDSIZE_3P125 0x500000 > >> > >> #define WDENABLE_KEY 0xa98559da > >> > >> @@ -48,7 +52,7 @@ > >> > >> #define DWDST BIT(1) > >> > >> -static int heartbeat; > >> +static int heartbeat = DEFAULT_HEARTBEAT; > >> > >> /* > >> * struct to hold data for each WDT device > >> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd) > >> * be petted during the open window; not too early or not too late. > >> * The HW configuration options only allow for the open window size > >> * to be 50% or less than that; we obviouly want to configure the open > >> - * window as large as possible so we select the 50% option. To avoid > >> - * any glitches, we accommodate 5% safety margin also, so we setup > >> - * the min_hw_hearbeat at 55% of the timeout period. > >> + * window as large as possible so we select the 50% option. > >> */ > >> - wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20; > >> + wdd->min_hw_heartbeat_ms = 500 * wdd->timeout; > >> > >> /* Generate NMI when wdt expires */ > >> writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL); > >> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd) > >> return 0; > >> } > >> > >> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > >> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize) > >> +{ > >> + /* > >> + * RTI only supports a windowed mode, where the watchdog can only > >> + * be petted during the open window; not too early or not too late. > >> + * The HW configuration options only allow for the open window size > >> + * to be 50% or less than that. > >> + */ > >> + switch (wsize) { > >> + case RTIWWDSIZE_50P: > >> + /* 50% open window => 50% min heartbeat */ > >> + wdd->min_hw_heartbeat_ms = 500 * heartbeat; > >> + break; > >> + > >> + case RTIWWDSIZE_25P: > >> + /* 25% open window => 75% min heartbeat */ > >> + wdd->min_hw_heartbeat_ms = 750 * heartbeat; > >> + break; > >> + > >> + case RTIWWDSIZE_12P5: > >> + /* 12.5% open window => 87.5% min heartbeat */ > >> + wdd->min_hw_heartbeat_ms = 875 * heartbeat; > >> + break; > >> + > >> + case RTIWWDSIZE_6P25: > >> + /* 6.5% open window => 93.5% min heartbeat */ > >> + wdd->min_hw_heartbeat_ms = 935 * heartbeat; > >> + break; > >> + > >> + case RTIWWDSIZE_3P125: > >> + /* 3.125% open window => 96.9% min heartbeat */ > >> + wdd->min_hw_heartbeat_ms = 969 * heartbeat; > >> + break; > >> + > >> + default: > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) > >> { > >> u64 timer_counter; > >> u32 val; > >> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > >> > >> timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR); > >> > >> + timer_counter *= 1000; > >> + > >> do_div(timer_counter, wdt->freq); > >> > >> return timer_counter; > >> } > >> > >> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > >> +{ > >> + return rti_wdt_get_timeleft_ms(wdd) / 1000; > >> +} > >> + > >> static const struct watchdog_info rti_wdt_info = { > >> .options = WDIOF_KEEPALIVEPING, > >> .identity = "K3 RTI Watchdog", > >> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev) > >> struct watchdog_device *wdd; > >> struct rti_wdt_device *wdt; > >> struct clk *clk; > >> + u32 last_ping = 0; > >> > >> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > >> if (!wdt) > >> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev) > >> return -EINVAL; > >> } > >> > >> + /* > >> + * If watchdog is running at 32k clock, it is not accurate. > >> + * Adjust frequency down in this case so that we don't pet > >> + * the watchdog too often. > >> + */ > >> + if (wdt->freq < 32768) > >> + wdt->freq = wdt->freq * 9 / 10; > >> + > > > > This seems broken: You are only adjusting the frequency value used by > > the driver. What has been programmed into the hardware already is still > > based on real frequency. Moreover, this path is not only taken when the > > watchdog is already running - but the latter case is what the subject > > and commit message suggests. I've noticed this by comparing > > bootloader-programmed values with those the driver assumes to see - 10% > > off, obviously. > > > > So, what is actually supposed to happen here? This assumes that the clk is coming in from RC_OSC_32k - which is in the range of accuracy of 10-20% off clock. also one more variable to keep in mind is that the 32k divided clk from hfosc will not be exact. Hari: Thoughts? > > > > Jan > > > >> pm_runtime_enable(dev); > >> ret = pm_runtime_get_sync(dev); > >> if (ret) { > >> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev) > >> wdd->min_timeout = 1; > >> wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / > >> wdt->freq * 1000; > >> - wdd->timeout = DEFAULT_HEARTBEAT; > >> wdd->parent = dev; > >> > >> - watchdog_init_timeout(wdd, heartbeat, dev); > >> - > >> watchdog_set_drvdata(wdd, wdt); > >> watchdog_set_nowayout(wdd, 1); > >> watchdog_set_restart_priority(wdd, 128); > >> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev) > >> goto err_iomap; > >> } > >> > >> + if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { > >> + u32 time_left_ms; > >> + u64 heartbeat_ms; > >> + u32 wsize; > >> + > >> + set_bit(WDOG_HW_RUNNING, &wdd->status); > >> + time_left_ms = rti_wdt_get_timeleft_ms(wdd); > >> + heartbeat_ms = readl(wdt->base + RTIDWDPRLD); > >> + heartbeat_ms <<= WDT_PRELOAD_SHIFT; > >> + heartbeat_ms *= 1000; > >> + do_div(heartbeat_ms, wdt->freq); > >> + if (heartbeat_ms != heartbeat * 1000) > >> + dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); > >> + > >> + heartbeat = heartbeat_ms; > >> + heartbeat /= 1000; > >> + > >> + wsize = readl(wdt->base + RTIWWDSIZECTRL); > >> + ret = rti_wdt_setup_hw_hb(wdd, wsize); > >> + if (ret) { > >> + dev_err(dev, "bad window size.\n"); > >> + goto err_iomap; > >> + } > >> + > >> + last_ping = heartbeat_ms - time_left_ms; > >> + if (time_left_ms > heartbeat_ms) { > >> + dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n"); > >> + last_ping = 0; > >> + } > >> + } > >> + > >> + watchdog_init_timeout(wdd, heartbeat, dev); > >> + > >> ret = watchdog_register_device(wdd); > >> if (ret) { > >> dev_err(dev, "cannot register watchdog device\n"); > >> goto err_iomap; > >> } > >> > >> + if (last_ping) > >> + watchdog_set_last_hw_keepalive(wdd, last_ping); > >> + > >> return 0; > >> > >> err_iomap: > > > > There is actually more "inaccurate". For now, I would try to address it > like this: > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index 88815419ad1a..1b6629fa5bfc 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -231,14 +231,6 @@ static int rti_wdt_probe(struct platform_device *pdev) > return -EINVAL; > } > > - /* > - * If watchdog is running at 32k clock, it is not accurate. > - * Adjust frequency down in this case so that we don't pet > - * the watchdog too often. > - */ > - if (wdt->freq < 32768) > - wdt->freq = wdt->freq * 9 / 10; > - > pm_runtime_enable(dev); > ret = pm_runtime_get_sync(dev); > if (ret) { > @@ -252,8 +244,6 @@ static int rti_wdt_probe(struct platform_device *pdev) > wdd->info = &rti_wdt_info; > wdd->ops = &rti_wdt_ops; > wdd->min_timeout = 1; > - wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / > - wdt->freq * 1000; > wdd->parent = dev; > > watchdog_set_drvdata(wdd, wdt); > @@ -280,7 +270,7 @@ static int rti_wdt_probe(struct platform_device *pdev) > if (heartbeat_ms != heartbeat * 1000) > dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); > > - heartbeat = heartbeat_ms; > + heartbeat = heartbeat_ms + 500; > heartbeat /= 1000; > > wsize = readl(wdt->base + RTIWWDSIZECTRL); > @@ -297,6 +287,17 @@ static int rti_wdt_probe(struct platform_device *pdev) > } > } > > + /* > + * If watchdog is running at 32k clock, it is not accurate. > + * Adjust frequency down in this case so that we don't pet > + * the watchdog too often. > + */ > + if (wdt->freq < 32768) > + wdt->freq = wdt->freq * 9 / 10; > + > + wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / > + wdt->freq * 1000; > + > watchdog_init_timeout(wdd, heartbeat, dev); > > ret = watchdog_register_device(wdd); > > > This moves the virtual frequency tweaking after reading back the > programmed timeout. It also properly rounds that up to full seconds so > that, e.g., bootloader-programmed programmed 60s will become 59s in the > driver. That could have led to too short min_hw_heartbeat_ms. > > I can send this out as real patch, but I'd still like to understand the > freq fiddling first. Is that addressing a real hardware issue? Or was it > actually papering over that round-up problem? > > Jan > > -- > Siemens AG, Technology > Competence Center Embedded Linux -- Regards, Nishanth Menon Key (0xDDB5849D1736249D)/Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe 2022-02-21 12:44 ` Nishanth Menon @ 2022-02-21 12:53 ` Jan Kiszka 2022-02-21 16:03 ` Jan Kiszka 0 siblings, 1 reply; 13+ messages in thread From: Jan Kiszka @ 2022-02-21 12:53 UTC (permalink / raw) To: Nishanth Menon, Hari Nagalla; +Cc: wim, linux, linux-watchdog, linux-kernel On 21.02.22 13:44, Nishanth Menon wrote: > On 10:59-20220221, Jan Kiszka wrote: >> On 21.02.22 10:10, Jan Kiszka wrote: >>> On 17.07.20 15:29, Tero Kristo wrote: >>>> If the RTI watchdog is running already during probe, the driver must >>>> configure itself to match the HW. Window size and timeout is probed from >>>> hardware, and the last keepalive ping is adjusted to match it also. >>>> >>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>> --- >>>> drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++---- >>>> 1 file changed, 102 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c >>>> index d456dd72d99a..7cbdc178ffe8 100644 >>>> --- a/drivers/watchdog/rti_wdt.c >>>> +++ b/drivers/watchdog/rti_wdt.c >>>> @@ -35,7 +35,11 @@ >>>> >>>> #define RTIWWDRX_NMI 0xa >>>> >>>> -#define RTIWWDSIZE_50P 0x50 >>>> +#define RTIWWDSIZE_50P 0x50 >>>> +#define RTIWWDSIZE_25P 0x500 >>>> +#define RTIWWDSIZE_12P5 0x5000 >>>> +#define RTIWWDSIZE_6P25 0x50000 >>>> +#define RTIWWDSIZE_3P125 0x500000 >>>> >>>> #define WDENABLE_KEY 0xa98559da >>>> >>>> @@ -48,7 +52,7 @@ >>>> >>>> #define DWDST BIT(1) >>>> >>>> -static int heartbeat; >>>> +static int heartbeat = DEFAULT_HEARTBEAT; >>>> >>>> /* >>>> * struct to hold data for each WDT device >>>> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd) >>>> * be petted during the open window; not too early or not too late. >>>> * The HW configuration options only allow for the open window size >>>> * to be 50% or less than that; we obviouly want to configure the open >>>> - * window as large as possible so we select the 50% option. To avoid >>>> - * any glitches, we accommodate 5% safety margin also, so we setup >>>> - * the min_hw_hearbeat at 55% of the timeout period. >>>> + * window as large as possible so we select the 50% option. >>>> */ >>>> - wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20; >>>> + wdd->min_hw_heartbeat_ms = 500 * wdd->timeout; >>>> >>>> /* Generate NMI when wdt expires */ >>>> writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL); >>>> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd) >>>> return 0; >>>> } >>>> >>>> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >>>> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize) >>>> +{ >>>> + /* >>>> + * RTI only supports a windowed mode, where the watchdog can only >>>> + * be petted during the open window; not too early or not too late. >>>> + * The HW configuration options only allow for the open window size >>>> + * to be 50% or less than that. >>>> + */ >>>> + switch (wsize) { >>>> + case RTIWWDSIZE_50P: >>>> + /* 50% open window => 50% min heartbeat */ >>>> + wdd->min_hw_heartbeat_ms = 500 * heartbeat; >>>> + break; >>>> + >>>> + case RTIWWDSIZE_25P: >>>> + /* 25% open window => 75% min heartbeat */ >>>> + wdd->min_hw_heartbeat_ms = 750 * heartbeat; >>>> + break; >>>> + >>>> + case RTIWWDSIZE_12P5: >>>> + /* 12.5% open window => 87.5% min heartbeat */ >>>> + wdd->min_hw_heartbeat_ms = 875 * heartbeat; >>>> + break; >>>> + >>>> + case RTIWWDSIZE_6P25: >>>> + /* 6.5% open window => 93.5% min heartbeat */ >>>> + wdd->min_hw_heartbeat_ms = 935 * heartbeat; >>>> + break; >>>> + >>>> + case RTIWWDSIZE_3P125: >>>> + /* 3.125% open window => 96.9% min heartbeat */ >>>> + wdd->min_hw_heartbeat_ms = 969 * heartbeat; >>>> + break; >>>> + >>>> + default: >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) >>>> { >>>> u64 timer_counter; >>>> u32 val; >>>> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >>>> >>>> timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR); >>>> >>>> + timer_counter *= 1000; >>>> + >>>> do_div(timer_counter, wdt->freq); >>>> >>>> return timer_counter; >>>> } >>>> >>>> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >>>> +{ >>>> + return rti_wdt_get_timeleft_ms(wdd) / 1000; >>>> +} >>>> + >>>> static const struct watchdog_info rti_wdt_info = { >>>> .options = WDIOF_KEEPALIVEPING, >>>> .identity = "K3 RTI Watchdog", >>>> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev) >>>> struct watchdog_device *wdd; >>>> struct rti_wdt_device *wdt; >>>> struct clk *clk; >>>> + u32 last_ping = 0; >>>> >>>> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); >>>> if (!wdt) >>>> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev) >>>> return -EINVAL; >>>> } >>>> >>>> + /* >>>> + * If watchdog is running at 32k clock, it is not accurate. >>>> + * Adjust frequency down in this case so that we don't pet >>>> + * the watchdog too often. >>>> + */ >>>> + if (wdt->freq < 32768) >>>> + wdt->freq = wdt->freq * 9 / 10; >>>> + >>> >>> This seems broken: You are only adjusting the frequency value used by >>> the driver. What has been programmed into the hardware already is still >>> based on real frequency. Moreover, this path is not only taken when the >>> watchdog is already running - but the latter case is what the subject >>> and commit message suggests. I've noticed this by comparing >>> bootloader-programmed values with those the driver assumes to see - 10% >>> off, obviously. >>> >>> So, what is actually supposed to happen here? > > > > This assumes that the clk is coming in from RC_OSC_32k - which is in the > range of accuracy of 10-20% off clock. also one more variable to keep in > mind is that the 32k divided clk from hfosc will not be exact. > OK, so we do want a safety margin for min_hw_heartbeat_ms, make it larger. But I still don't think it is best achieved by bending the frequency. That will also affect other values, e.g. returning a wrong programmed timeout to userspace if that was programmed earlier, using the original frequency. > Hari: Thoughts? > >>> >>> Jan >>> >>>> pm_runtime_enable(dev); >>>> ret = pm_runtime_get_sync(dev); >>>> if (ret) { >>>> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev) >>>> wdd->min_timeout = 1; >>>> wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / >>>> wdt->freq * 1000; >>>> - wdd->timeout = DEFAULT_HEARTBEAT; >>>> wdd->parent = dev; >>>> >>>> - watchdog_init_timeout(wdd, heartbeat, dev); >>>> - >>>> watchdog_set_drvdata(wdd, wdt); >>>> watchdog_set_nowayout(wdd, 1); >>>> watchdog_set_restart_priority(wdd, 128); >>>> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev) >>>> goto err_iomap; >>>> } >>>> >>>> + if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { >>>> + u32 time_left_ms; >>>> + u64 heartbeat_ms; >>>> + u32 wsize; >>>> + >>>> + set_bit(WDOG_HW_RUNNING, &wdd->status); >>>> + time_left_ms = rti_wdt_get_timeleft_ms(wdd); >>>> + heartbeat_ms = readl(wdt->base + RTIDWDPRLD); >>>> + heartbeat_ms <<= WDT_PRELOAD_SHIFT; >>>> + heartbeat_ms *= 1000; >>>> + do_div(heartbeat_ms, wdt->freq); >>>> + if (heartbeat_ms != heartbeat * 1000) >>>> + dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); >>>> + >>>> + heartbeat = heartbeat_ms; >>>> + heartbeat /= 1000; >>>> + >>>> + wsize = readl(wdt->base + RTIWWDSIZECTRL); >>>> + ret = rti_wdt_setup_hw_hb(wdd, wsize); >>>> + if (ret) { >>>> + dev_err(dev, "bad window size.\n"); >>>> + goto err_iomap; >>>> + } >>>> + >>>> + last_ping = heartbeat_ms - time_left_ms; >>>> + if (time_left_ms > heartbeat_ms) { >>>> + dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n"); >>>> + last_ping = 0; >>>> + } >>>> + } >>>> + >>>> + watchdog_init_timeout(wdd, heartbeat, dev); >>>> + >>>> ret = watchdog_register_device(wdd); >>>> if (ret) { >>>> dev_err(dev, "cannot register watchdog device\n"); >>>> goto err_iomap; >>>> } >>>> >>>> + if (last_ping) >>>> + watchdog_set_last_hw_keepalive(wdd, last_ping); >>>> + >>>> return 0; >>>> >>>> err_iomap: >>> >> >> There is actually more "inaccurate". For now, I would try to address it >> like this: >> >> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c >> index 88815419ad1a..1b6629fa5bfc 100644 >> --- a/drivers/watchdog/rti_wdt.c >> +++ b/drivers/watchdog/rti_wdt.c >> @@ -231,14 +231,6 @@ static int rti_wdt_probe(struct platform_device *pdev) >> return -EINVAL; >> } >> >> - /* >> - * If watchdog is running at 32k clock, it is not accurate. >> - * Adjust frequency down in this case so that we don't pet >> - * the watchdog too often. >> - */ >> - if (wdt->freq < 32768) >> - wdt->freq = wdt->freq * 9 / 10; >> - >> pm_runtime_enable(dev); >> ret = pm_runtime_get_sync(dev); >> if (ret) { >> @@ -252,8 +244,6 @@ static int rti_wdt_probe(struct platform_device *pdev) >> wdd->info = &rti_wdt_info; >> wdd->ops = &rti_wdt_ops; >> wdd->min_timeout = 1; >> - wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / >> - wdt->freq * 1000; >> wdd->parent = dev; >> >> watchdog_set_drvdata(wdd, wdt); >> @@ -280,7 +270,7 @@ static int rti_wdt_probe(struct platform_device *pdev) >> if (heartbeat_ms != heartbeat * 1000) >> dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); >> >> - heartbeat = heartbeat_ms; >> + heartbeat = heartbeat_ms + 500; >> heartbeat /= 1000; >> >> wsize = readl(wdt->base + RTIWWDSIZECTRL); >> @@ -297,6 +287,17 @@ static int rti_wdt_probe(struct platform_device *pdev) >> } >> } >> >> + /* >> + * If watchdog is running at 32k clock, it is not accurate. >> + * Adjust frequency down in this case so that we don't pet >> + * the watchdog too often. >> + */ >> + if (wdt->freq < 32768) >> + wdt->freq = wdt->freq * 9 / 10; >> + >> + wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / >> + wdt->freq * 1000; >> + >> watchdog_init_timeout(wdd, heartbeat, dev); >> >> ret = watchdog_register_device(wdd); >> >> >> This moves the virtual frequency tweaking after reading back the >> programmed timeout. It also properly rounds that up to full seconds so >> that, e.g., bootloader-programmed programmed 60s will become 59s in the >> driver. That could have led to too short min_hw_heartbeat_ms. >> >> I can send this out as real patch, but I'd still like to understand the >> freq fiddling first. Is that addressing a real hardware issue? Or was it >> actually papering over that round-up problem? >> >> Jan >> >> -- >> Siemens AG, Technology >> Competence Center Embedded Linux > Jan -- Siemens AG, Technology Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe 2022-02-21 12:53 ` Jan Kiszka @ 2022-02-21 16:03 ` Jan Kiszka 2022-02-21 16:29 ` Hari Nagalla 0 siblings, 1 reply; 13+ messages in thread From: Jan Kiszka @ 2022-02-21 16:03 UTC (permalink / raw) To: Nishanth Menon, Hari Nagalla; +Cc: wim, linux, linux-watchdog, linux-kernel On 21.02.22 13:53, Jan Kiszka wrote: > On 21.02.22 13:44, Nishanth Menon wrote: >> On 10:59-20220221, Jan Kiszka wrote: >>> On 21.02.22 10:10, Jan Kiszka wrote: >>>> On 17.07.20 15:29, Tero Kristo wrote: >>>>> If the RTI watchdog is running already during probe, the driver must >>>>> configure itself to match the HW. Window size and timeout is probed from >>>>> hardware, and the last keepalive ping is adjusted to match it also. >>>>> >>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com> >>>>> --- >>>>> drivers/watchdog/rti_wdt.c | 112 +++++++++++++++++++++++++++++++++---- >>>>> 1 file changed, 102 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c >>>>> index d456dd72d99a..7cbdc178ffe8 100644 >>>>> --- a/drivers/watchdog/rti_wdt.c >>>>> +++ b/drivers/watchdog/rti_wdt.c >>>>> @@ -35,7 +35,11 @@ >>>>> >>>>> #define RTIWWDRX_NMI 0xa >>>>> >>>>> -#define RTIWWDSIZE_50P 0x50 >>>>> +#define RTIWWDSIZE_50P 0x50 >>>>> +#define RTIWWDSIZE_25P 0x500 >>>>> +#define RTIWWDSIZE_12P5 0x5000 >>>>> +#define RTIWWDSIZE_6P25 0x50000 >>>>> +#define RTIWWDSIZE_3P125 0x500000 >>>>> >>>>> #define WDENABLE_KEY 0xa98559da >>>>> >>>>> @@ -48,7 +52,7 @@ >>>>> >>>>> #define DWDST BIT(1) >>>>> >>>>> -static int heartbeat; >>>>> +static int heartbeat = DEFAULT_HEARTBEAT; >>>>> >>>>> /* >>>>> * struct to hold data for each WDT device >>>>> @@ -79,11 +83,9 @@ static int rti_wdt_start(struct watchdog_device *wdd) >>>>> * be petted during the open window; not too early or not too late. >>>>> * The HW configuration options only allow for the open window size >>>>> * to be 50% or less than that; we obviouly want to configure the open >>>>> - * window as large as possible so we select the 50% option. To avoid >>>>> - * any glitches, we accommodate 5% safety margin also, so we setup >>>>> - * the min_hw_hearbeat at 55% of the timeout period. >>>>> + * window as large as possible so we select the 50% option. >>>>> */ >>>>> - wdd->min_hw_heartbeat_ms = 11 * wdd->timeout * 1000 / 20; >>>>> + wdd->min_hw_heartbeat_ms = 500 * wdd->timeout; >>>>> >>>>> /* Generate NMI when wdt expires */ >>>>> writel_relaxed(RTIWWDRX_NMI, wdt->base + RTIWWDRXCTRL); >>>>> @@ -110,7 +112,48 @@ static int rti_wdt_ping(struct watchdog_device *wdd) >>>>> return 0; >>>>> } >>>>> >>>>> -static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >>>>> +static int rti_wdt_setup_hw_hb(struct watchdog_device *wdd, u32 wsize) >>>>> +{ >>>>> + /* >>>>> + * RTI only supports a windowed mode, where the watchdog can only >>>>> + * be petted during the open window; not too early or not too late. >>>>> + * The HW configuration options only allow for the open window size >>>>> + * to be 50% or less than that. >>>>> + */ >>>>> + switch (wsize) { >>>>> + case RTIWWDSIZE_50P: >>>>> + /* 50% open window => 50% min heartbeat */ >>>>> + wdd->min_hw_heartbeat_ms = 500 * heartbeat; >>>>> + break; >>>>> + >>>>> + case RTIWWDSIZE_25P: >>>>> + /* 25% open window => 75% min heartbeat */ >>>>> + wdd->min_hw_heartbeat_ms = 750 * heartbeat; >>>>> + break; >>>>> + >>>>> + case RTIWWDSIZE_12P5: >>>>> + /* 12.5% open window => 87.5% min heartbeat */ >>>>> + wdd->min_hw_heartbeat_ms = 875 * heartbeat; >>>>> + break; >>>>> + >>>>> + case RTIWWDSIZE_6P25: >>>>> + /* 6.5% open window => 93.5% min heartbeat */ >>>>> + wdd->min_hw_heartbeat_ms = 935 * heartbeat; >>>>> + break; >>>>> + >>>>> + case RTIWWDSIZE_3P125: >>>>> + /* 3.125% open window => 96.9% min heartbeat */ >>>>> + wdd->min_hw_heartbeat_ms = 969 * heartbeat; >>>>> + break; >>>>> + >>>>> + default: >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) >>>>> { >>>>> u64 timer_counter; >>>>> u32 val; >>>>> @@ -123,11 +166,18 @@ static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >>>>> >>>>> timer_counter = readl_relaxed(wdt->base + RTIDWDCNTR); >>>>> >>>>> + timer_counter *= 1000; >>>>> + >>>>> do_div(timer_counter, wdt->freq); >>>>> >>>>> return timer_counter; >>>>> } >>>>> >>>>> +static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) >>>>> +{ >>>>> + return rti_wdt_get_timeleft_ms(wdd) / 1000; >>>>> +} >>>>> + >>>>> static const struct watchdog_info rti_wdt_info = { >>>>> .options = WDIOF_KEEPALIVEPING, >>>>> .identity = "K3 RTI Watchdog", >>>>> @@ -148,6 +198,7 @@ static int rti_wdt_probe(struct platform_device *pdev) >>>>> struct watchdog_device *wdd; >>>>> struct rti_wdt_device *wdt; >>>>> struct clk *clk; >>>>> + u32 last_ping = 0; >>>>> >>>>> wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); >>>>> if (!wdt) >>>>> @@ -169,6 +220,14 @@ static int rti_wdt_probe(struct platform_device *pdev) >>>>> return -EINVAL; >>>>> } >>>>> >>>>> + /* >>>>> + * If watchdog is running at 32k clock, it is not accurate. >>>>> + * Adjust frequency down in this case so that we don't pet >>>>> + * the watchdog too often. >>>>> + */ >>>>> + if (wdt->freq < 32768) >>>>> + wdt->freq = wdt->freq * 9 / 10; >>>>> + >>>> >>>> This seems broken: You are only adjusting the frequency value used by >>>> the driver. What has been programmed into the hardware already is still >>>> based on real frequency. Moreover, this path is not only taken when the >>>> watchdog is already running - but the latter case is what the subject >>>> and commit message suggests. I've noticed this by comparing >>>> bootloader-programmed values with those the driver assumes to see - 10% >>>> off, obviously. >>>> >>>> So, what is actually supposed to happen here? >> >> >> >> This assumes that the clk is coming in from RC_OSC_32k - which is in the >> range of accuracy of 10-20% off clock. also one more variable to keep in >> mind is that the 32k divided clk from hfosc will not be exact. >> > > OK, so we do want a safety margin for min_hw_heartbeat_ms, make it > larger. But I still don't think it is best achieved by bending the > frequency. That will also affect other values, e.g. returning a wrong > programmed timeout to userspace if that was programmed earlier, using > the original frequency. > I think I'm starting to get the original logic, and the result now works here: The clock driving the watchdog might be slower than thought, and then we may time out later than intended - generally not an issue. But it may also be faster, and then we will see an expiry earlier than what is supposed to be configured via "heartbeat". For the latter case, we lower the frequency virtually by 10%, crossing fingers that this is enough. The problems are now: - U-Boot (as a known early watchdog starter) does not do that as well, and we will cause at least confusion on Linux side (60s will become 66s from Linux POV e.g., and we may expire at 54s already) => U-Boot should add the same 10%, patch will be sent - even with U-Boot on the same page, the rounding issue will prevent accurate calculations of derived values, namely min_hw_heartbeat_ms. => patch to come - and ... >> Hari: Thoughts? >> >>>> >>>> Jan >>>> >>>>> pm_runtime_enable(dev); >>>>> ret = pm_runtime_get_sync(dev); >>>>> if (ret) { >>>>> @@ -185,11 +244,8 @@ static int rti_wdt_probe(struct platform_device *pdev) >>>>> wdd->min_timeout = 1; >>>>> wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / >>>>> wdt->freq * 1000; >>>>> - wdd->timeout = DEFAULT_HEARTBEAT; >>>>> wdd->parent = dev; >>>>> >>>>> - watchdog_init_timeout(wdd, heartbeat, dev); >>>>> - >>>>> watchdog_set_drvdata(wdd, wdt); >>>>> watchdog_set_nowayout(wdd, 1); >>>>> watchdog_set_restart_priority(wdd, 128); >>>>> @@ -201,12 +257,48 @@ static int rti_wdt_probe(struct platform_device *pdev) >>>>> goto err_iomap; >>>>> } >>>>> >>>>> + if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) { >>>>> + u32 time_left_ms; >>>>> + u64 heartbeat_ms; >>>>> + u32 wsize; >>>>> + >>>>> + set_bit(WDOG_HW_RUNNING, &wdd->status); >>>>> + time_left_ms = rti_wdt_get_timeleft_ms(wdd); >>>>> + heartbeat_ms = readl(wdt->base + RTIDWDPRLD); >>>>> + heartbeat_ms <<= WDT_PRELOAD_SHIFT; >>>>> + heartbeat_ms *= 1000; >>>>> + do_div(heartbeat_ms, wdt->freq); >>>>> + if (heartbeat_ms != heartbeat * 1000) >>>>> + dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); ...this comparison is not ok, leading to a practically impossible to fulfill condition. Will fix that up while adding the round-up. Jan >>>>> + >>>>> + heartbeat = heartbeat_ms; >>>>> + heartbeat /= 1000; >>>>> + >>>>> + wsize = readl(wdt->base + RTIWWDSIZECTRL); >>>>> + ret = rti_wdt_setup_hw_hb(wdd, wsize); >>>>> + if (ret) { >>>>> + dev_err(dev, "bad window size.\n"); >>>>> + goto err_iomap; >>>>> + } >>>>> + >>>>> + last_ping = heartbeat_ms - time_left_ms; >>>>> + if (time_left_ms > heartbeat_ms) { >>>>> + dev_warn(dev, "time_left > heartbeat? Assuming last ping just before now.\n"); >>>>> + last_ping = 0; >>>>> + } >>>>> + } >>>>> + >>>>> + watchdog_init_timeout(wdd, heartbeat, dev); >>>>> + >>>>> ret = watchdog_register_device(wdd); >>>>> if (ret) { >>>>> dev_err(dev, "cannot register watchdog device\n"); >>>>> goto err_iomap; >>>>> } >>>>> >>>>> + if (last_ping) >>>>> + watchdog_set_last_hw_keepalive(wdd, last_ping); >>>>> + >>>>> return 0; >>>>> >>>>> err_iomap: >>>> >>> >>> There is actually more "inaccurate". For now, I would try to address it >>> like this: >>> >>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c >>> index 88815419ad1a..1b6629fa5bfc 100644 >>> --- a/drivers/watchdog/rti_wdt.c >>> +++ b/drivers/watchdog/rti_wdt.c >>> @@ -231,14 +231,6 @@ static int rti_wdt_probe(struct platform_device *pdev) >>> return -EINVAL; >>> } >>> >>> - /* >>> - * If watchdog is running at 32k clock, it is not accurate. >>> - * Adjust frequency down in this case so that we don't pet >>> - * the watchdog too often. >>> - */ >>> - if (wdt->freq < 32768) >>> - wdt->freq = wdt->freq * 9 / 10; >>> - >>> pm_runtime_enable(dev); >>> ret = pm_runtime_get_sync(dev); >>> if (ret) { >>> @@ -252,8 +244,6 @@ static int rti_wdt_probe(struct platform_device *pdev) >>> wdd->info = &rti_wdt_info; >>> wdd->ops = &rti_wdt_ops; >>> wdd->min_timeout = 1; >>> - wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / >>> - wdt->freq * 1000; >>> wdd->parent = dev; >>> >>> watchdog_set_drvdata(wdd, wdt); >>> @@ -280,7 +270,7 @@ static int rti_wdt_probe(struct platform_device *pdev) >>> if (heartbeat_ms != heartbeat * 1000) >>> dev_warn(dev, "watchdog already running, ignoring heartbeat config!\n"); >>> >>> - heartbeat = heartbeat_ms; >>> + heartbeat = heartbeat_ms + 500; >>> heartbeat /= 1000; >>> >>> wsize = readl(wdt->base + RTIWWDSIZECTRL); >>> @@ -297,6 +287,17 @@ static int rti_wdt_probe(struct platform_device *pdev) >>> } >>> } >>> >>> + /* >>> + * If watchdog is running at 32k clock, it is not accurate. >>> + * Adjust frequency down in this case so that we don't pet >>> + * the watchdog too often. >>> + */ >>> + if (wdt->freq < 32768) >>> + wdt->freq = wdt->freq * 9 / 10; >>> + >>> + wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) / >>> + wdt->freq * 1000; >>> + >>> watchdog_init_timeout(wdd, heartbeat, dev); >>> >>> ret = watchdog_register_device(wdd); >>> >>> >>> This moves the virtual frequency tweaking after reading back the >>> programmed timeout. It also properly rounds that up to full seconds so >>> that, e.g., bootloader-programmed programmed 60s will become 59s in the >>> driver. That could have led to too short min_hw_heartbeat_ms. >>> >>> I can send this out as real patch, but I'd still like to understand the >>> freq fiddling first. Is that addressing a real hardware issue? Or was it >>> actually papering over that round-up problem? >>> >>> Jan >>> >>> -- >>> Siemens AG, Technology >>> Competence Center Embedded Linux >> > > Jan > -- Siemens AG, Technology Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe 2022-02-21 16:03 ` Jan Kiszka @ 2022-02-21 16:29 ` Hari Nagalla 0 siblings, 0 replies; 13+ messages in thread From: Hari Nagalla @ 2022-02-21 16:29 UTC (permalink / raw) To: Jan Kiszka, Nishanth Menon; +Cc: wim, linux, linux-watchdog, linux-kernel On 2/21/22 10:03, Jan Kiszka wrote: >> K, so we do want a safety margin for min_hw_heartbeat_ms, make it >> larger. But I still don't think it is best achieved by bending the >> frequency. That will also affect other values, e.g. returning a wrong >> programmed timeout to userspace if that was programmed earlier, using >> the original frequency. >> > I think I'm starting to get the original logic, and the result now works > here: > > The clock driving the watchdog might be slower than thought, and then we > may time out later than intended - generally not an issue. But it may > also be faster, and then we will see an expiry earlier than what is > supposed to be configured via "heartbeat". For the latter case, we lower > the frequency virtually by 10%, crossing fingers that this is enough. > Humm.. To me it appears the intent is to adjust when the input 32KHz clock is slower? when it is slower we reduce the pulse count by 10% (assuming the crystals are with 10% off clock) so that the desired timeout is achieved with lesser pulse count? > The problems are now: > - U-Boot (as a known early watchdog starter) does not do that as well, > and we will cause at least confusion on Linux side (60s will become > 66s from Linux POV e.g., and we may expire at 54s already) > => U-Boot should add the same 10%, patch will be sent Yes, i see that we need similar adjustment in u-boot as well. > - even with U-Boot on the same page, the rounding issue will prevent > accurate calculations of derived values, namely min_hw_heartbeat_ms. > => patch to come > - and ... > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv4 4/4] watchdog: rti-wdt: balance pm runtime enable calls 2020-07-17 13:29 [PATCHv4 0/4] watchdog: rti-wdt: attach to running timer Tero Kristo ` (2 preceding siblings ...) 2020-07-17 13:29 ` [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe Tero Kristo @ 2020-07-17 13:29 ` Tero Kristo 3 siblings, 0 replies; 13+ messages in thread From: Tero Kristo @ 2020-07-17 13:29 UTC (permalink / raw) To: wim, linux, linux-watchdog; +Cc: linux-kernel, jan.kiszka PM runtime should be disabled in the fail path of probe and when the driver is removed. Fixes: 2d63908bdbfb ("watchdog: Add K3 RTI watchdog support") Signed-off-by: Tero Kristo <t-kristo@ti.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> --- drivers/watchdog/rti_wdt.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c index 7cbdc178ffe8..705e8f7523e8 100644 --- a/drivers/watchdog/rti_wdt.c +++ b/drivers/watchdog/rti_wdt.c @@ -303,6 +303,7 @@ static int rti_wdt_probe(struct platform_device *pdev) err_iomap: pm_runtime_put_sync(&pdev->dev); + pm_runtime_disable(&pdev->dev); return ret; } @@ -313,6 +314,7 @@ static int rti_wdt_remove(struct platform_device *pdev) watchdog_unregister_device(&wdt->wdd); pm_runtime_put(&pdev->dev); + pm_runtime_disable(&pdev->dev); return 0; } -- 2.17.1 -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-02-21 16:29 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-17 13:29 [PATCHv4 0/4] watchdog: rti-wdt: attach to running timer Tero Kristo 2020-07-17 13:29 ` [PATCHv4 1/4] watchdog: use __watchdog_ping in startup Tero Kristo 2020-07-17 13:29 ` [PATCHv4 2/4] watchdog: add support for adjusting last known HW keepalive time Tero Kristo 2020-07-19 13:54 ` Guenter Roeck 2020-07-17 13:29 ` [PATCHv4 3/4] watchdog: rti-wdt: attach to running watchdog during probe Tero Kristo 2020-07-19 13:56 ` Guenter Roeck 2022-02-21 9:10 ` Jan Kiszka 2022-02-21 9:59 ` Jan Kiszka 2022-02-21 12:44 ` Nishanth Menon 2022-02-21 12:53 ` Jan Kiszka 2022-02-21 16:03 ` Jan Kiszka 2022-02-21 16:29 ` Hari Nagalla 2020-07-17 13:29 ` [PATCHv4 4/4] watchdog: rti-wdt: balance pm runtime enable calls Tero Kristo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).