diff mbox series

[v2,1/2] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

Message ID 20240216210258.24855-2-pohsuns@nvidia.com
State New
Headers
Series clocksource: fix Tegra234 SoC Watchdog Timer. |

Commit Message

Pohsun Su Feb. 16, 2024, 9:02 p.m. UTC 
  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 <pohsuns@nvidia.com>
---
 drivers/clocksource/timer-tegra186.c | 44 +++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

Comments

Thierry Reding Feb. 19, 2024, 4:06 p.m. UTC | #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 <pohsuns@nvidia.com>
> ---
>  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,
Thierry Reding Feb. 19, 2024, 4:18 p.m. UTC | #2 
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
Pohsun Su Feb. 23, 2024, 11:51 p.m. UTC | #3 
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
Thierry Reding Feb. 27, 2024, 11:58 a.m. UTC | #4 
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
diff mbox series

Patch

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,