This set of patches includes a fix for watchdog for it may not bark
due to self-pinging and adds WDIOC_GETTIMELEFT support.
--
Patchset v2 fixes a compilation error, a warning and updates copyright:
drivers/clocksource/timer-tegra186.c:263:22: error:
implicit declaration of function 'FIELD_GET'
[-Werror=implicit-function-declaration]
drivers/clocksource/timer-tegra186.c:414:15: warning:
variable 'irq' set but not used [-Wunused-but-set-variable]
--
Differences between v2 and v1:
[PATCH v2 1/2] clocksource/drivers/timer-tegra186: add
WDIOC_GETTIMELEFT support
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Copyright (c) 2019-2020 NVIDIA Corporation. All rights reserved.
+ * Copyright (c) 2019-2024 NVIDIA Corporation. All rights reserved.
*/
+#include <linux/bitfield.h>
#include <linux/clocksource.h>
#include <linux/module.h>
#include <linux/interrupt.h>
[PATCH v2 2/2] clocksource/drivers/timer-tegra186: fix watchdog
self-pinging.
struct tegra186_timer *tegra;
- unsigned int irq;
int err;
tegra = devm_kzalloc(dev, sizeof(*tegra), GFP_KERNEL);
if (err < 0)
return err;
- irq = err;
-
/* create a watchdog using a preconfigured timer */
tegra->wdt = tegra186_wdt_create(tegra, 0);
if (IS_ERR(tegra->wdt)) {
Pohsun Su (2):
clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support
clocksource/drivers/timer-tegra186: fix watchdog self-pinging.
drivers/clocksource/timer-tegra186.c | 71 ++++++++++++++++++----------
1 file changed, 45 insertions(+), 26 deletions(-)
--
2.17.1
This change adds support for WDIOC_GETTIMELEFT so userspace
programs can get the number of seconds before system reset by
the watchdog timer via ioctl.
Signed-off-by: Pohsun Su <[email protected]>
---
drivers/clocksource/timer-tegra186.c | 44 +++++++++++++++++++++++++++-
1 file changed, 43 insertions(+), 1 deletion(-)
diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
index 304537dadf2c..8f516366da86 100644
--- a/drivers/clocksource/timer-tegra186.c
+++ b/drivers/clocksource/timer-tegra186.c
@@ -1,8 +1,9 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * Copyright (c) 2019-2020 NVIDIA Corporation. All rights reserved.
+ * Copyright (c) 2019-2024 NVIDIA Corporation. All rights reserved.
*/
+#include <linux/bitfield.h>
#include <linux/clocksource.h>
#include <linux/module.h>
#include <linux/interrupt.h>
@@ -29,6 +30,7 @@
#define TMRSR 0x004
#define TMRSR_INTR_CLR BIT(30)
+#define TMRSR_PCV GENMASK(28, 0)
#define TMRCSSR 0x008
#define TMRCSSR_SRC_USEC (0 << 0)
@@ -45,6 +47,9 @@
#define WDTCR_TIMER_SOURCE_MASK 0xf
#define WDTCR_TIMER_SOURCE(x) ((x) & 0xf)
+#define WDTSR 0x004
+#define WDTSR_CURRENT_EXPIRATION_COUNT GENMASK(14, 12)
+
#define WDTCMDR 0x008
#define WDTCMDR_DISABLE_COUNTER BIT(1)
#define WDTCMDR_START_COUNTER BIT(0)
@@ -234,12 +239,49 @@ static int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
return 0;
}
+static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+ struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
+ u32 timeleft;
+ u32 expiration;
+
+ if (!watchdog_active(&wdt->base)) {
+ /* return zero if the watchdog timer is not activated. */
+ return 0;
+ }
+
+ /*
+ * System power-on reset occurs on the fifth expiration of the watchdog timer and so
+ * when the watchdog timer is configured, the actual value programmed into the counter
+ * is 1/5 of the timeout value. Once the counter reaches 0, expiration count will be
+ * increased by 1 and the down counter restarts.
+ * Hence to get the time left before system reset we must combine 2 parts:
+ * 1. value of the current down counter
+ * 2. (number of counter expirations remaining) * (timeout/5)
+ */
+
+ /* Get the current number of counter expirations. Should be a value between 0 and 4. */
+ expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, readl_relaxed(wdt->regs + WDTSR));
+
+ /* Convert the current counter value to seconds, rounding up to the nearest second. */
+ timeleft = FIELD_GET(TMRSR_PCV, readl_relaxed(wdt->tmr->regs + TMRSR));
+ timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
+
+ /*
+ * Calculate the time remaining by adding the time for the counter value
+ * to the time of the counter expirations that remain.
+ */
+ timeleft += wdt->base.timeout * (4 - expiration) / 5;
+ return timeleft;
+}
+
static const struct watchdog_ops tegra186_wdt_ops = {
.owner = THIS_MODULE,
.start = tegra186_wdt_start,
.stop = tegra186_wdt_stop,
.ping = tegra186_wdt_ping,
.set_timeout = tegra186_wdt_set_timeout,
+ .get_timeleft = tegra186_wdt_get_timeleft,
};
static struct tegra186_wdt *tegra186_wdt_create(struct tegra186_timer *tegra,
--
2.17.1
This change removes watchdog self-pinging behavior.
The timer irq handler is triggered due to the 1st expiration,
the handler disables and enables watchdog but also implicitly
clears the expiration count so the count can only be 0 or 1.
Since this watchdog supports opened, configured, or pinged by
systemd, We remove this behavior or the watchdog may not bark
when systemd crashes since the 5th expiration never comes.
Signed-off-by: Pohsun Su <[email protected]>
---
drivers/clocksource/timer-tegra186.c | 27 ++-------------------------
1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
index 8f516366da86..acff97da138a 100644
--- a/drivers/clocksource/timer-tegra186.c
+++ b/drivers/clocksource/timer-tegra186.c
@@ -175,7 +175,8 @@ static void tegra186_wdt_enable(struct tegra186_wdt *wdt)
value |= WDTCR_PERIOD(1);
/* enable local interrupt for WDT petting */
- value |= WDTCR_LOCAL_INT_ENABLE;
+ if (0)
+ value |= WDTCR_LOCAL_INT_ENABLE;
/* enable local FIQ and remote interrupt for debug dump */
if (0)
@@ -407,23 +408,10 @@ static int tegra186_timer_usec_init(struct tegra186_timer *tegra)
return clocksource_register_hz(&tegra->usec, USEC_PER_SEC);
}
-static irqreturn_t tegra186_timer_irq(int irq, void *data)
-{
- struct tegra186_timer *tegra = data;
-
- if (watchdog_active(&tegra->wdt->base)) {
- tegra186_wdt_disable(tegra->wdt);
- tegra186_wdt_enable(tegra->wdt);
- }
-
- return IRQ_HANDLED;
-}
-
static int tegra186_timer_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct tegra186_timer *tegra;
- unsigned int irq;
int err;
tegra = devm_kzalloc(dev, sizeof(*tegra), GFP_KERNEL);
@@ -442,8 +430,6 @@ static int tegra186_timer_probe(struct platform_device *pdev)
if (err < 0)
return err;
- irq = err;
-
/* create a watchdog using a preconfigured timer */
tegra->wdt = tegra186_wdt_create(tegra, 0);
if (IS_ERR(tegra->wdt)) {
@@ -470,17 +456,8 @@ static int tegra186_timer_probe(struct platform_device *pdev)
goto unregister_osc;
}
- err = devm_request_irq(dev, irq, tegra186_timer_irq, 0,
- "tegra186-timer", tegra);
- if (err < 0) {
- dev_err(dev, "failed to request IRQ#%u: %d\n", irq, err);
- goto unregister_usec;
- }
-
return 0;
-unregister_usec:
- clocksource_unregister(&tegra->usec);
unregister_osc:
clocksource_unregister(&tegra->osc);
unregister_tsc:
--
2.17.1
On Fri Feb 16, 2024 at 10:02 PM CET, Pohsun Su wrote:
> This change adds support for WDIOC_GETTIMELEFT so userspace
> programs can get the number of seconds before system reset by
> the watchdog timer via ioctl.
>
> Signed-off-by: Pohsun Su <[email protected]>
> ---
> drivers/clocksource/timer-tegra186.c | 44 +++++++++++++++++++++++++++-
> 1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> index 304537dadf2c..8f516366da86 100644
> --- a/drivers/clocksource/timer-tegra186.c
> +++ b/drivers/clocksource/timer-tegra186.c
> @@ -1,8 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> - * Copyright (c) 2019-2020 NVIDIA Corporation. All rights reserved.
> + * Copyright (c) 2019-2024 NVIDIA Corporation. All rights reserved.
> */
>
> +#include <linux/bitfield.h>
> #include <linux/clocksource.h>
> #include <linux/module.h>
> #include <linux/interrupt.h>
> @@ -29,6 +30,7 @@
>
> #define TMRSR 0x004
> #define TMRSR_INTR_CLR BIT(30)
> +#define TMRSR_PCV GENMASK(28, 0)
>
> #define TMRCSSR 0x008
> #define TMRCSSR_SRC_USEC (0 << 0)
> @@ -45,6 +47,9 @@
> #define WDTCR_TIMER_SOURCE_MASK 0xf
> #define WDTCR_TIMER_SOURCE(x) ((x) & 0xf)
>
> +#define WDTSR 0x004
> +#define WDTSR_CURRENT_EXPIRATION_COUNT GENMASK(14, 12)
> +
> #define WDTCMDR 0x008
> #define WDTCMDR_DISABLE_COUNTER BIT(1)
> #define WDTCMDR_START_COUNTER BIT(0)
> @@ -234,12 +239,49 @@ static int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
> return 0;
> }
>
> +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> + struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> + u32 timeleft;
> + u32 expiration;
> +
> + if (!watchdog_active(&wdt->base)) {
> + /* return zero if the watchdog timer is not activated. */
> + return 0;
> + }
> +
> + /*
> + * System power-on reset occurs on the fifth expiration of the watchdog timer and so
Is "system power-on reset" really what this is called? Power-on reset
sounds like something that only happens after you power the device on,
not something that can be triggered by the watchdog.
> + * when the watchdog timer is configured, the actual value programmed into the counter
> + * is 1/5 of the timeout value. Once the counter reaches 0, expiration count will be
> + * increased by 1 and the down counter restarts.
> + * Hence to get the time left before system reset we must combine 2 parts:
> + * 1. value of the current down counter
> + * 2. (number of counter expirations remaining) * (timeout/5)
> + */
Can you wrap this comment so that it fits within 80 columns? It's fine
to occasionally go beyond that limit if there's a good reason for it,
but this comment doesn't seem to fall into that category.
> +
> + /* Get the current number of counter expirations. Should be a value between 0 and 4. */
> + expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, readl_relaxed(wdt->regs + WDTSR));
> +
> + /* Convert the current counter value to seconds, rounding up to the nearest second. */
> + timeleft = FIELD_GET(TMRSR_PCV, readl_relaxed(wdt->tmr->regs + TMRSR));
> + timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
Same for these. Maybe make an extra variable to store the register value
in to get rid of some of that extra horizontal space.
> +
> + /*
> + * Calculate the time remaining by adding the time for the counter value
> + * to the time of the counter expirations that remain.
> + */
> + timeleft += wdt->base.timeout * (4 - expiration) / 5;
This doesn't quite match what the comment above says. Shouldn't this be:
timeleft += (wdt->base.timeout / 5) * (4 - expiration);
instead?
Thierry
> + return timeleft;
> +}
> +
> static const struct watchdog_ops tegra186_wdt_ops = {
> .owner = THIS_MODULE,
> .start = tegra186_wdt_start,
> .stop = tegra186_wdt_stop,
> .ping = tegra186_wdt_ping,
> .set_timeout = tegra186_wdt_set_timeout,
> + .get_timeleft = tegra186_wdt_get_timeleft,
> };
>
> static struct tegra186_wdt *tegra186_wdt_create(struct tegra186_timer *tegra,
On Fri Feb 16, 2024 at 10:02 PM CET, Pohsun Su wrote:
> This change removes watchdog self-pinging behavior.
>
> The timer irq handler is triggered due to the 1st expiration,
> the handler disables and enables watchdog but also implicitly
> clears the expiration count so the count can only be 0 or 1.
>
> Since this watchdog supports opened, configured, or pinged by
> systemd, We remove this behavior or the watchdog may not bark
> when systemd crashes since the 5th expiration never comes.
>
> Signed-off-by: Pohsun Su <[email protected]>
> ---
> drivers/clocksource/timer-tegra186.c | 27 ++-------------------------
> 1 file changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> index 8f516366da86..acff97da138a 100644
> --- a/drivers/clocksource/timer-tegra186.c
> +++ b/drivers/clocksource/timer-tegra186.c
> @@ -175,7 +175,8 @@ static void tegra186_wdt_enable(struct tegra186_wdt *wdt)
> value |= WDTCR_PERIOD(1);
>
> /* enable local interrupt for WDT petting */
> - value |= WDTCR_LOCAL_INT_ENABLE;
> + if (0)
> + value |= WDTCR_LOCAL_INT_ENABLE;
We probably shouldn't proliferate this scheme. In retrospect I should've
removed the two other similar blocks back when I submitted the driver at
the time since they don't really serve a purpose. The intention at the
time was to keep them there and eventually replace the condition with
something that could actually be toggled, but it's been almost four
years and this hasn't happened, so I suspect that we just don't need it
at all. So perhaps you could remove this line along with the comment in
this patch and then add another patch that removes the other unused bits
so that we don't carry around stuff that we just never use.
Thierry
On Fri Feb 16, 2024 at 10:02 PM CET, Pohsun Su wrote:
[...]
> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
[...]
> @@ -234,12 +239,49 @@ static int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
> return 0;
> }
>
> +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> + struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> + u32 timeleft;
> + u32 expiration;
One more thing I noticed: you could put both of these declarations on a
single line to make this a bit more compact.
Thierry
> On Fri Feb 16, 2024 at 10:02 PM CET, Pohsun Su wrote:
>> This change removes watchdog self-pinging behavior.
>>
>> The timer irq handler is triggered due to the 1st expiration,
>> the handler disables and enables watchdog but also implicitly
>> clears the expiration count so the count can only be 0 or 1.
>>
>> Since this watchdog supports opened, configured, or pinged by
>> systemd, We remove this behavior or the watchdog may not bark
>> when systemd crashes since the 5th expiration never comes.
>>
>> Signed-off-by: Pohsun Su <[email protected]>
>> ---
>> drivers/clocksource/timer-tegra186.c | 27 ++-------------------------
>> 1 file changed, 2 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
>> index 8f516366da86..acff97da138a 100644
>> --- a/drivers/clocksource/timer-tegra186.c
>> +++ b/drivers/clocksource/timer-tegra186.c
>> @@ -175,7 +175,8 @@ static void tegra186_wdt_enable(struct tegra186_wdt *wdt)
>> value |= WDTCR_PERIOD(1);
>>
>> /* enable local interrupt for WDT petting */
>> - value |= WDTCR_LOCAL_INT_ENABLE;
>> + if (0)
>> + value |= WDTCR_LOCAL_INT_ENABLE;
>
> We probably shouldn't proliferate this scheme. In retrospect I should've
> removed the two other similar blocks back when I submitted the driver at
> the time since they don't really serve a purpose. The intention at the
> time was to keep them there and eventually replace the condition with
> something that could actually be toggled, but it's been almost four
> years and this hasn't happened, so I suspect that we just don't need it
> at all. So perhaps you could remove this line along with the comment in
> this patch and then add another patch that removes the other unused bits
> so that we don't carry around stuff that we just never use.
>
> Thierry
Sure, removing both lines and the comment above.
will add another patch to clean unused bits.
Thanks!
--
Pohsun
Hi Thierry,
>> +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd)
>> +{
>> + struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
>> + u32 timeleft;
>> + u32 expiration;
>> +
>> + if (!watchdog_active(&wdt->base)) {
>> + /* return zero if the watchdog timer is not activated. */
>> + return 0;
>> + }
>> +
>> + /*
>> + * System power-on reset occurs on the fifth expiration of the watchdog timer and so
>
>Is "system power-on reset" really what this is called? Power-on reset
>sounds like something that only happens after you power the device on,
>not something that can be triggered by the watchdog.
I will change it from "system power-on reset" to "System POR(Power On Reset)" in next patch.
AFAIK Power On Reset is used for decribing resetting circuits and initialing whatever it needs
when received a POR signal after powered up. This term should also be applicable for
hardware watchdog reset since the system is already powered up at that moment and
>> + * when the watchdog timer is configured, the actual value programmed into the counter
>> + * is 1/5 of the timeout value. Once the counter reaches 0, expiration count will be
>> + * increased by 1 and the down counter restarts.
>> + * Hence to get the time left before system reset we must combine 2 parts:
>> + * 1. value of the current down counter
>> + * 2. (number of counter expirations remaining) * (timeout/5)
>> + */
>
>Can you wrap this comment so that it fits within 80 columns? It's fine
>to occasionally go beyond that limit if there's a good reason for it,
>but this comment doesn't seem to fall into that category.
Sorry, I missed to check the line length before submit,
will wrap comments in next patch.
>> +
>> + /* Get the current number of counter expirations. Should be a value between 0 and 4. */
>> + expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, readl_relaxed(wdt->regs + WDTSR));
>> +
>> + /* Convert the current counter value to seconds, rounding up to the nearest second. */
>> + timeleft = FIELD_GET(TMRSR_PCV, readl_relaxed(wdt->tmr->regs + TMRSR));
>> + timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
>
>Same for these. Maybe make an extra variable to store the register value
>in to get rid of some of that extra horizontal space.
Thanks for the suggestion, will do.
>> +
>> + /*
>> + * Calculate the time remaining by adding the time for the counter value
>> + * to the time of the counter expirations that remain.
>> + */
>> + timeleft += wdt->base.timeout * (4 - expiration) / 5;
>
>This doesn't quite match what the comment above says. Shouldn't this be:
>
> timeleft += (wdt->base.timeout / 5) * (4 - expiration);
>
>instead?
Here I made a transposition on purpose just for keeping the precision due to the integer division.
e.g. If 'divided by 5' goes first, the equation goes to 0 when wdt->base.timeout is less than 5
no matter which timer expiration it is.
But the number will be still inaccurate because I made a mistake that
the number should be calculated in microseconds all the time
and be converted to a nearest second only in the last step.
A new patch has been made and I will submit it after verifications.
Thank you for your time on reviewing this!
Best,
--
Pohsun
On Sat Feb 24, 2024 at 12:51 AM CET, Pohsun Su wrote:
> Hi Thierry,
>
> >> +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wdd)
> >> +{
> >> + struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> >> + u32 timeleft;
> >> + u32 expiration;
> >> +
> >> + if (!watchdog_active(&wdt->base)) {
> >> + /* return zero if the watchdog timer is not activated. */
> >> + return 0;
> >> + }
> >> +
> >> + /*
> >> + * System power-on reset occurs on the fifth expiration of the watchdog timer and so
> >
> >Is "system power-on reset" really what this is called? Power-on reset
> >sounds like something that only happens after you power the device on,
> >not something that can be triggered by the watchdog.
>
> I will change it from "system power-on reset" to "System POR(Power On Reset)" in next patch.
> AFAIK Power On Reset is used for decribing resetting circuits and initialing whatever it needs
> when received a POR signal after powered up. This term should also be applicable for
> hardware watchdog reset since the system is already powered up at that moment and
"System POR" isn't an improvement over "system power-on reset". I'm
mainly concerned that somebody might mistake this to somehow mean that
there's an actual power cycle, which, as I understand, there isn't. So
maybe just explain that this type of reset will put the system into the
same state that it would be after a power cycle?
Thierry