[PATCH v7 0/3] clocksource: fix Tegra234 SoC Watchdog Timer.

[PATCH v7 0/3] clocksource: fix Tegra234 SoC Watchdog Timer.

[Robert Lin]

From: robelin <robelin@nvidia.com>

This set of patches includes a fix for watchdog for it may not bark
due to self-pinging and adds WDIOC_GETTIMELEFT support.

--
V7:
- Fix formatting
- Consider overflow and warn if happens

V6:
- Fix timeleft value addition using unmatched time unit
- Use u64 type to maintain the microseconds value in case of overflow

V5:
- Print warning message if get unexpected value from the register

V4:
- Improve the precision of timeleft value
- Fix the unused variable warning

V3:
- Improve comment description
- Refactor to fit codeline within 80 columns
- Remove unused if(0) blocks


V2:
- Fix a compilation error, a warning and updates copyright
--

Pohsun Su (2):
  clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support
  clocksource/drivers/timer-tegra186: fix watchdog self-pinging

robelin (1):
  clocksource/drivers/timer-tegra186: Remove unused bits

 drivers/clocksource/timer-tegra186.c | 100 +++++++++++++++++----------
 1 file changed, 63 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Robert Lin]

From: Pohsun Su <pohsuns@nvidia.com>

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>
Signed-off-by: Robert Lin <robelin@nvidia.com>
---
 drivers/clocksource/timer-tegra186.c | 64 +++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
index ea742889ee06..8d5698770cbd 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-2025 NVIDIA Corporation. All rights reserved.
  */
 
+#include <linux/bitfield.h>
 #include <linux/clocksource.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
@@ -30,6 +31,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)
@@ -46,6 +48,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)
@@ -235,12 +240,69 @@ 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 expiration, val;
+	u64 timeleft;
+
+	if (!watchdog_active(&wdt->base)) {
+		/* return zero if the watchdog timer is not activated. */
+		return 0;
+	}
+
+	/*
+	 * 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
+	 */
+	val = readl_relaxed(wdt->regs + WDTSR);
+	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
+	if (WARN_ON(expiration > 4))
+		return 0;
+
+	/* Get the current counter value in microsecond. */
+	val = readl_relaxed(wdt->tmr->regs + TMRSR);
+	timeleft = FIELD_GET(TMRSR_PCV, val);
+
+	/*
+	 * Calculate the time remaining by adding the time for the
+	 * counter value to the time of the counter expirations that
+	 * remain.
+	 */
+	timeleft += (((u64)wdt->base.timeout * USEC_PER_SEC) / 5) * (4 - expiration);
+
+	/*
+	 * Convert the current counter value to seconds,
+	 * rounding up to the nearest second. Cast u64 to
+	 * u32 under the assumption that no overflow happens
+	 * when coverting to seconds.
+	 */
+	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
+
+	if (WARN_ON(timeleft > U32_MAX))
+		return U32_MAX;
+
+	return lower_32_bits(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.34.1

[PATCH v7 2/3] clocksource/drivers/timer-tegra186: fix watchdog self-pinging

[Robert Lin]

From: Pohsun Su <pohsuns@nvidia.com>

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 <pohsuns@nvidia.com>
Signed-off-by: Robert Lin <robelin@nvidia.com>
---
 drivers/clocksource/timer-tegra186.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
index 8d5698770cbd..8c8d83ceb1dd 100644
--- a/drivers/clocksource/timer-tegra186.c
+++ b/drivers/clocksource/timer-tegra186.c
@@ -175,9 +175,6 @@ static void tegra186_wdt_enable(struct tegra186_wdt *wdt)
 		value &= ~WDTCR_PERIOD_MASK;
 		value |= WDTCR_PERIOD(1);
 
-		/* enable local interrupt for WDT petting */
-		value |= WDTCR_LOCAL_INT_ENABLE;
-
 		/* enable local FIQ and remote interrupt for debug dump */
 		if (0)
 			value |= WDTCR_REMOTE_INT_ENABLE |
@@ -428,23 +425,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);
@@ -463,8 +447,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)) {
@@ -491,17 +473,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.34.1

[PATCH v7 3/3] clocksource/drivers/timer-tegra186: Remove unused bits

[Robert Lin]

From: robelin <robelin@nvidia.com>

The intention to keep the unsed if(0) block is gone now. Remove
them for clean codes.

Signed-off-by: robelin <robelin@nvidia.com>
---
 drivers/clocksource/timer-tegra186.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
index 8c8d83ceb1dd..afbf180c1baf 100644
--- a/drivers/clocksource/timer-tegra186.c
+++ b/drivers/clocksource/timer-tegra186.c
@@ -175,15 +175,6 @@ static void tegra186_wdt_enable(struct tegra186_wdt *wdt)
 		value &= ~WDTCR_PERIOD_MASK;
 		value |= WDTCR_PERIOD(1);
 
-		/* enable local FIQ and remote interrupt for debug dump */
-		if (0)
-			value |= WDTCR_REMOTE_INT_ENABLE |
-				 WDTCR_LOCAL_FIQ_ENABLE;
-
-		/* enable system debug reset (doesn't properly reboot) */
-		if (0)
-			value |= WDTCR_SYSTEM_DEBUG_RESET_ENABLE;
-
 		/* enable system POR reset */
 		value |= WDTCR_SYSTEM_POR_RESET_ENABLE;
 
-- 
2.34.1

Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Daniel Lezcano]

On Fri, May 02, 2025 at 12:37:25PM +0800, Robert Lin wrote:
> From: Pohsun Su <pohsuns@nvidia.com>
> 
> 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>
> Signed-off-by: Robert Lin <robelin@nvidia.com>
> ---
>  drivers/clocksource/timer-tegra186.c | 64 +++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> index ea742889ee06..8d5698770cbd 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-2025 NVIDIA Corporation. All rights reserved.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clocksource.h>
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
> @@ -30,6 +31,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)
> @@ -46,6 +48,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)
> @@ -235,12 +240,69 @@ 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 expiration, val;
> +	u64 timeleft;
> +
> +	if (!watchdog_active(&wdt->base)) {
> +		/* return zero if the watchdog timer is not activated. */
> +		return 0;
> +	}
> +
> +	/*
> +	 * 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
> +	 */
> +	val = readl_relaxed(wdt->regs + WDTSR);
> +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
> +	if (WARN_ON(expiration > 4))
> +		return 0;

Each call will generate a big warning in the message. May be simpler
to add a pr_err() with an explicit message.

> +	/* Get the current counter value in microsecond. */
> +	val = readl_relaxed(wdt->tmr->regs + TMRSR);
> +	timeleft = FIELD_GET(TMRSR_PCV, val);
> +
> +	/*
> +	 * Calculate the time remaining by adding the time for the
> +	 * counter value to the time of the counter expirations that
> +	 * remain.
> +	 */
> +	timeleft += (((u64)wdt->base.timeout * USEC_PER_SEC) / 5) * (4 - expiration);
> +
> +	/*
> +	 * Convert the current counter value to seconds,
> +	 * rounding up to the nearest second. Cast u64 to
> +	 * u32 under the assumption that no overflow happens
> +	 * when coverting to seconds.
> +	 */
> +	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;

Did you check there is a macro fitting the need in math.h ?

> +	if (WARN_ON(timeleft > U32_MAX))

s/WARN_ON/pr_err/

> +		return U32_MAX;
> +
> +	return lower_32_bits(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.34.1
> 

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v7 0/3] clocksource: fix Tegra234 SoC Watchdog Timer.

[Daniel Lezcano]

On Fri, May 02, 2025 at 12:37:24PM +0800, Robert Lin wrote:
> From: robelin <robelin@nvidia.com>
> 
> This set of patches includes a fix for watchdog for it may not bark
> due to self-pinging and adds WDIOC_GETTIMELEFT support.
> 
> --

When sending a new version, could you please Cc: the watchdog maintainers ?

Thanks
  -- Daniel

> V7:
> - Fix formatting
> - Consider overflow and warn if happens
> 
> V6:
> - Fix timeleft value addition using unmatched time unit
> - Use u64 type to maintain the microseconds value in case of overflow
> 
> V5:
> - Print warning message if get unexpected value from the register
> 
> V4:
> - Improve the precision of timeleft value
> - Fix the unused variable warning
> 
> V3:
> - Improve comment description
> - Refactor to fit codeline within 80 columns
> - Remove unused if(0) blocks
> 
> 
> V2:
> - Fix a compilation error, a warning and updates copyright
> --

[ ... ]

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Jon Hunter]

On 02/05/2025 10:19, Daniel Lezcano wrote:
> On Fri, May 02, 2025 at 12:37:25PM +0800, Robert Lin wrote:
>> From: Pohsun Su <pohsuns@nvidia.com>
>>
>> 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>
>> Signed-off-by: Robert Lin <robelin@nvidia.com>
>> ---
>>   drivers/clocksource/timer-tegra186.c | 64 +++++++++++++++++++++++++++-
>>   1 file changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
>> index ea742889ee06..8d5698770cbd 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-2025 NVIDIA Corporation. All rights reserved.
>>    */
>>   
>> +#include <linux/bitfield.h>
>>   #include <linux/clocksource.h>
>>   #include <linux/module.h>
>>   #include <linux/interrupt.h>
>> @@ -30,6 +31,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)
>> @@ -46,6 +48,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)
>> @@ -235,12 +240,69 @@ 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 expiration, val;
>> +	u64 timeleft;
>> +
>> +	if (!watchdog_active(&wdt->base)) {
>> +		/* return zero if the watchdog timer is not activated. */
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * 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
>> +	 */
>> +	val = readl_relaxed(wdt->regs + WDTSR);
>> +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
>> +	if (WARN_ON(expiration > 4))
>> +		return 0;
> 
> Each call will generate a big warning in the message. May be simpler
> to add a pr_err() with an explicit message.

I prefer the WARN. This should never happen. If we do change this, then 
dev_WARN() might be more appropriate, but I think that this is fine too.

> 
>> +	/* Get the current counter value in microsecond. */
>> +	val = readl_relaxed(wdt->tmr->regs + TMRSR);
>> +	timeleft = FIELD_GET(TMRSR_PCV, val);
>> +
>> +	/*
>> +	 * Calculate the time remaining by adding the time for the
>> +	 * counter value to the time of the counter expirations that
>> +	 * remain.
>> +	 */
>> +	timeleft += (((u64)wdt->base.timeout * USEC_PER_SEC) / 5) * (4 - expiration);
>> +
>> +	/*
>> +	 * Convert the current counter value to seconds,
>> +	 * rounding up to the nearest second. Cast u64 to
>> +	 * u32 under the assumption that no overflow happens
>> +	 * when coverting to seconds.
>> +	 */
>> +	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
> 
> Did you check there is a macro fitting the need in math.h ?

I had a quick look, but looking again, I see we can use 
DIV_ROUND_CLOSEST_ULL() here.

> 
>> +	if (WARN_ON(timeleft > U32_MAX))
> 
> s/WARN_ON/pr_err/

Why? Again this is not expected. I think that this is fine.

> 
>> +		return U32_MAX;
>> +
>> +	return lower_32_bits(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.34.1
>>
> 

-- 
nvpublic

Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Daniel Lezcano]

On Fri, May 02, 2025 at 11:16:17AM +0100, Jon Hunter wrote:
> 
> 
> On 02/05/2025 10:19, Daniel Lezcano wrote:
> > On Fri, May 02, 2025 at 12:37:25PM +0800, Robert Lin wrote:
> > > From: Pohsun Su <pohsuns@nvidia.com>
> > > 
> > > 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>
> > > Signed-off-by: Robert Lin <robelin@nvidia.com>
> > > ---
> > >   drivers/clocksource/timer-tegra186.c | 64 +++++++++++++++++++++++++++-
> > >   1 file changed, 63 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> > > index ea742889ee06..8d5698770cbd 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-2025 NVIDIA Corporation. All rights reserved.
> > >    */
> > > +#include <linux/bitfield.h>
> > >   #include <linux/clocksource.h>
> > >   #include <linux/module.h>
> > >   #include <linux/interrupt.h>
> > > @@ -30,6 +31,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)
> > > @@ -46,6 +48,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)
> > > @@ -235,12 +240,69 @@ 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 expiration, val;
> > > +	u64 timeleft;
> > > +
> > > +	if (!watchdog_active(&wdt->base)) {
> > > +		/* return zero if the watchdog timer is not activated. */
> > > +		return 0;
> > > +	}
> > > +
> > > +	/*
> > > +	 * 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
> > > +	 */
> > > +	val = readl_relaxed(wdt->regs + WDTSR);
> > > +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
> > > +	if (WARN_ON(expiration > 4))
> > > +		return 0;
> > 
> > Each call will generate a big warning in the message. May be simpler
> > to add a pr_err() with an explicit message.
> 
> I prefer the WARN. This should never happen. If we do change this, then
> dev_WARN() might be more appropriate, but I think that this is fine too.

The function tegra186_wdt_get_timeleft() is triggered from userspace
via an ioctl or sysfs. The documentation process/coding-style.rst says:

"""
Do not WARN lightly
*******************

WARN*() is intended for unexpected, this-should-never-happen situations.
WARN*() macros are not to be used for anything that is expected to happen
during normal operation. These are not pre- or post-condition asserts, for
example. Again: WARN*() must not be used for a condition that is expected
to trigger easily, for example, by user space actions. pr_warn_once() is a
possible alternative, if you need to notify the user of a problem.
"""

I understand it is important to check the return value in order to
have a consistent result when computing the remaining time but that
should not trigger a warning each time.

[ ... ]

> > > +	/*
> > > +	 * Convert the current counter value to seconds,
> > > +	 * rounding up to the nearest second. Cast u64 to
> > > +	 * u32 under the assumption that no overflow happens
> > > +	 * when coverting to seconds.
> > > +	 */
> > > +	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
> > 
> > Did you check there is a macro fitting the need in math.h ?
> 
> I had a quick look, but looking again, I see we can use
> DIV_ROUND_CLOSEST_ULL() here.

What about 'roundup()' ?

> > > +	if (WARN_ON(timeleft > U32_MAX))
> > 
> > s/WARN_ON/pr_err/
> 
> Why? Again this is not expected. I think that this is fine.

[ ... ]

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Jon Hunter]

On 02/05/2025 11:51, Daniel Lezcano wrote:
> On Fri, May 02, 2025 at 11:16:17AM +0100, Jon Hunter wrote:
>>
>>
>> On 02/05/2025 10:19, Daniel Lezcano wrote:
>>> On Fri, May 02, 2025 at 12:37:25PM +0800, Robert Lin wrote:
>>>> From: Pohsun Su <pohsuns@nvidia.com>
>>>>
>>>> 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>
>>>> Signed-off-by: Robert Lin <robelin@nvidia.com>
>>>> ---
>>>>    drivers/clocksource/timer-tegra186.c | 64 +++++++++++++++++++++++++++-
>>>>    1 file changed, 63 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
>>>> index ea742889ee06..8d5698770cbd 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-2025 NVIDIA Corporation. All rights reserved.
>>>>     */
>>>> +#include <linux/bitfield.h>
>>>>    #include <linux/clocksource.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/interrupt.h>
>>>> @@ -30,6 +31,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)
>>>> @@ -46,6 +48,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)
>>>> @@ -235,12 +240,69 @@ 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 expiration, val;
>>>> +	u64 timeleft;
>>>> +
>>>> +	if (!watchdog_active(&wdt->base)) {
>>>> +		/* return zero if the watchdog timer is not activated. */
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * 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
>>>> +	 */
>>>> +	val = readl_relaxed(wdt->regs + WDTSR);
>>>> +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
>>>> +	if (WARN_ON(expiration > 4))
>>>> +		return 0;
>>>
>>> Each call will generate a big warning in the message. May be simpler
>>> to add a pr_err() with an explicit message.
>>
>> I prefer the WARN. This should never happen. If we do change this, then
>> dev_WARN() might be more appropriate, but I think that this is fine too.
> 
> The function tegra186_wdt_get_timeleft() is triggered from userspace
> via an ioctl or sysfs. The documentation process/coding-style.rst says:
> 
> """
> Do not WARN lightly
> *******************
> 
> WARN*() is intended for unexpected, this-should-never-happen situations.
> WARN*() macros are not to be used for anything that is expected to happen
> during normal operation. These are not pre- or post-condition asserts, for
> example. Again: WARN*() must not be used for a condition that is expected
> to trigger easily, for example, by user space actions. pr_warn_once() is a
> possible alternative, if you need to notify the user of a problem.
> """
> 
> I understand it is important to check the return value in order to
> have a consistent result when computing the remaining time but that
> should not trigger a warning each time.

Yes so WARN sounds appropriate. It should never happen. I don't see the 
issue.

>>>> +	/*
>>>> +	 * Convert the current counter value to seconds,
>>>> +	 * rounding up to the nearest second. Cast u64 to
>>>> +	 * u32 under the assumption that no overflow happens
>>>> +	 * when coverting to seconds.
>>>> +	 */
>>>> +	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
>>>
>>> Did you check there is a macro fitting the need in math.h ?
>>
>> I had a quick look, but looking again, I see we can use
>> DIV_ROUND_CLOSEST_ULL() here.
> 
> What about 'roundup()' ?

'roundup' does not look the same as what is being done above. However, 
DIV_ROUND_CLOSEST_ULL() does.

-- 
nvpublic

Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Daniel Lezcano]

On 02/05/2025 13:06, Jon Hunter wrote:
> 
> 
> On 02/05/2025 11:51, Daniel Lezcano wrote:
>> On Fri, May 02, 2025 at 11:16:17AM +0100, Jon Hunter wrote:
>>>
>>>
>>> On 02/05/2025 10:19, Daniel Lezcano wrote:
>>>> On Fri, May 02, 2025 at 12:37:25PM +0800, Robert Lin wrote:
>>>>> From: Pohsun Su <pohsuns@nvidia.com>
>>>>>
>>>>> 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>
>>>>> Signed-off-by: Robert Lin <robelin@nvidia.com>
>>>>> ---
>>>>>    drivers/clocksource/timer-tegra186.c | 64 ++++++++++++++++++++++ 
>>>>> +++++-
>>>>>    1 file changed, 63 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/ 
>>>>> clocksource/timer-tegra186.c
>>>>> index ea742889ee06..8d5698770cbd 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-2025 NVIDIA Corporation. All rights reserved.
>>>>>     */
>>>>> +#include <linux/bitfield.h>
>>>>>    #include <linux/clocksource.h>
>>>>>    #include <linux/module.h>
>>>>>    #include <linux/interrupt.h>
>>>>> @@ -30,6 +31,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)
>>>>> @@ -46,6 +48,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)
>>>>> @@ -235,12 +240,69 @@ 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 expiration, val;
>>>>> +    u64 timeleft;
>>>>> +
>>>>> +    if (!watchdog_active(&wdt->base)) {
>>>>> +        /* return zero if the watchdog timer is not activated. */
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * 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
>>>>> +     */
>>>>> +    val = readl_relaxed(wdt->regs + WDTSR);
>>>>> +    expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
>>>>> +    if (WARN_ON(expiration > 4))
>>>>> +        return 0;
>>>>
>>>> Each call will generate a big warning in the message. May be simpler
>>>> to add a pr_err() with an explicit message.
>>>
>>> I prefer the WARN. This should never happen. If we do change this, then
>>> dev_WARN() might be more appropriate, but I think that this is fine too.
>>
>> The function tegra186_wdt_get_timeleft() is triggered from userspace
>> via an ioctl or sysfs. The documentation process/coding-style.rst says:
>>
>> """
>> Do not WARN lightly
>> *******************
>>
>> WARN*() is intended for unexpected, this-should-never-happen situations.
>> WARN*() macros are not to be used for anything that is expected to happen
>> during normal operation. These are not pre- or post-condition asserts, 
>> for
>> example. Again: WARN*() must not be used for a condition that is expected
>> to trigger easily, for example, by user space actions. pr_warn_once() 
>> is a
>> possible alternative, if you need to notify the user of a problem.
>> """
>>
>> I understand it is important to check the return value in order to
>> have a consistent result when computing the remaining time but that
>> should not trigger a warning each time.
> 
> Yes so WARN sounds appropriate. It should never happen. I don't see the 
> issue.

The issue is if there is an userspace application reading the ioctl and 
or the sysfs, then the warning will be emitted each time if the 
never-happen condition exists. Preferably replace the WARN_ON by 
pr_warn_once() as suggested if the bug must be reported.

>>>>> +    /*
>>>>> +     * Convert the current counter value to seconds,
>>>>> +     * rounding up to the nearest second. Cast u64 to
>>>>> +     * u32 under the assumption that no overflow happens
>>>>> +     * when coverting to seconds.
>>>>> +     */
>>>>> +    timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
>>>>
>>>> Did you check there is a macro fitting the need in math.h ?
>>>
>>> I had a quick look, but looking again, I see we can use
>>> DIV_ROUND_CLOSEST_ULL() here.
>>
>> What about 'roundup()' ?
> 
> 'roundup' does not look the same as what is being done above. However, 
> DIV_ROUND_CLOSEST_ULL() does.

Ok, thanks for checking

   -- Daniel

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Jon Hunter]

On 02/05/2025 12:29, Daniel Lezcano wrote:
> On 02/05/2025 13:06, Jon Hunter wrote:
>>
>>
>> On 02/05/2025 11:51, Daniel Lezcano wrote:
>>> On Fri, May 02, 2025 at 11:16:17AM +0100, Jon Hunter wrote:
>>>>
>>>>
>>>> On 02/05/2025 10:19, Daniel Lezcano wrote:
>>>>> On Fri, May 02, 2025 at 12:37:25PM +0800, Robert Lin wrote:
>>>>>> From: Pohsun Su <pohsuns@nvidia.com>
>>>>>>
>>>>>> 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>
>>>>>> Signed-off-by: Robert Lin <robelin@nvidia.com>
>>>>>> ---
>>>>>>    drivers/clocksource/timer-tegra186.c | 64 +++++++++++++++++++++ 
>>>>>> + +++++-
>>>>>>    1 file changed, 63 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/ 
>>>>>> clocksource/timer-tegra186.c
>>>>>> index ea742889ee06..8d5698770cbd 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-2025 NVIDIA Corporation. All rights reserved.
>>>>>>     */
>>>>>> +#include <linux/bitfield.h>
>>>>>>    #include <linux/clocksource.h>
>>>>>>    #include <linux/module.h>
>>>>>>    #include <linux/interrupt.h>
>>>>>> @@ -30,6 +31,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)
>>>>>> @@ -46,6 +48,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)
>>>>>> @@ -235,12 +240,69 @@ 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 expiration, val;
>>>>>> +    u64 timeleft;
>>>>>> +
>>>>>> +    if (!watchdog_active(&wdt->base)) {
>>>>>> +        /* return zero if the watchdog timer is not activated. */
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +
>>>>>> +    /*
>>>>>> +     * 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
>>>>>> +     */
>>>>>> +    val = readl_relaxed(wdt->regs + WDTSR);
>>>>>> +    expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
>>>>>> +    if (WARN_ON(expiration > 4))
>>>>>> +        return 0;
>>>>>
>>>>> Each call will generate a big warning in the message. May be simpler
>>>>> to add a pr_err() with an explicit message.
>>>>
>>>> I prefer the WARN. This should never happen. If we do change this, then
>>>> dev_WARN() might be more appropriate, but I think that this is fine 
>>>> too.
>>>
>>> The function tegra186_wdt_get_timeleft() is triggered from userspace
>>> via an ioctl or sysfs. The documentation process/coding-style.rst says:
>>>
>>> """
>>> Do not WARN lightly
>>> *******************
>>>
>>> WARN*() is intended for unexpected, this-should-never-happen situations.
>>> WARN*() macros are not to be used for anything that is expected to 
>>> happen
>>> during normal operation. These are not pre- or post-condition 
>>> asserts, for
>>> example. Again: WARN*() must not be used for a condition that is 
>>> expected
>>> to trigger easily, for example, by user space actions. pr_warn_once() 
>>> is a
>>> possible alternative, if you need to notify the user of a problem.
>>> """
>>>
>>> I understand it is important to check the return value in order to
>>> have a consistent result when computing the remaining time but that
>>> should not trigger a warning each time.
>>
>> Yes so WARN sounds appropriate. It should never happen. I don't see 
>> the issue.
> 
> The issue is if there is an userspace application reading the ioctl and 
> or the sysfs, then the warning will be emitted each time if the never- 
> happen condition exists. Preferably replace the WARN_ON by 
> pr_warn_once() as suggested if the bug must be reported.

Sounds a bit funny 'if the never-happen condition exists' :-)

However, I will be fine with WARN_ON_ONCE(). I think that this warrants 
more of a large WARN splat than pr_warn() because it should never happen.

Jon

-- 
nvpublic

RE: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Robert Lin]

> -----Original Message-----
> From: Jon Hunter <jonathanh@nvidia.com>
> Sent: Friday, May 2, 2025 8:37 PM
> To: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Robert Lin <robelin@nvidia.com>; thierry.reding@gmail.com;
> tglx@linutronix.de; Pohsun Su <pohsuns@nvidia.com>; linux-
> kernel@vger.kernel.org; linux-tegra@vger.kernel.org; Sumit Gupta
> <sumitg@nvidia.com>
> Subject: Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add
> WDIOC_GETTIMELEFT support
> 
> 
> On 02/05/2025 12:29, Daniel Lezcano wrote:
> > On 02/05/2025 13:06, Jon Hunter wrote:
> >>
> >>
> >> On 02/05/2025 11:51, Daniel Lezcano wrote:
> >>> On Fri, May 02, 2025 at 11:16:17AM +0100, Jon Hunter wrote:
> >>>>
> >>>>
> >>>> On 02/05/2025 10:19, Daniel Lezcano wrote:
> >>>>> On Fri, May 02, 2025 at 12:37:25PM +0800, Robert Lin wrote:
> >>>>>> From: Pohsun Su <pohsuns@nvidia.com>
> >>>>>>
> >>>>>> 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>
> >>>>>> Signed-off-by: Robert Lin <robelin@nvidia.com>
> >>>>>> ---
> >>>>>>    drivers/clocksource/timer-tegra186.c | 64
> >>>>>> +++++++++++++++++++++
> >>>>>> + +++++-
> >>>>>>    1 file changed, 63 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/
> >>>>>> clocksource/timer-tegra186.c index ea742889ee06..8d5698770cbd
> >>>>>> 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-2025 NVIDIA Corporation. All rights reserved.
> >>>>>>     */
> >>>>>> +#include <linux/bitfield.h>
> >>>>>>    #include <linux/clocksource.h>
> >>>>>>    #include <linux/module.h>
> >>>>>>    #include <linux/interrupt.h>
> >>>>>> @@ -30,6 +31,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) @@ -46,6 +48,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) @@ -235,12 +240,69
> @@
> >>>>>> 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 expiration, val;
> >>>>>> +    u64 timeleft;
> >>>>>> +
> >>>>>> +    if (!watchdog_active(&wdt->base)) {
> >>>>>> +        /* return zero if the watchdog timer is not activated.
> >>>>>> +*/
> >>>>>> +        return 0;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    /*
> >>>>>> +     * 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
> >>>>>> +     */
> >>>>>> +    val = readl_relaxed(wdt->regs + WDTSR);
> >>>>>> +    expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT,
> val);
> >>>>>> +    if (WARN_ON(expiration > 4))
> >>>>>> +        return 0;
> >>>>>
> >>>>> Each call will generate a big warning in the message. May be
> >>>>> simpler to add a pr_err() with an explicit message.
> >>>>
> >>>> I prefer the WARN. This should never happen. If we do change this,
> >>>> then
> >>>> dev_WARN() might be more appropriate, but I think that this is fine
> >>>> too.
> >>>
> >>> The function tegra186_wdt_get_timeleft() is triggered from userspace
> >>> via an ioctl or sysfs. The documentation process/coding-style.rst says:
> >>>
> >>> """
> >>> Do not WARN lightly
> >>> *******************
> >>>
> >>> WARN*() is intended for unexpected, this-should-never-happen
> situations.
> >>> WARN*() macros are not to be used for anything that is expected to
> >>> happen during normal operation. These are not pre- or post-condition
> >>> asserts, for example. Again: WARN*() must not be used for a
> >>> condition that is expected to trigger easily, for example, by user
> >>> space actions. pr_warn_once() is a possible alternative, if you need
> >>> to notify the user of a problem.
> >>> """
> >>>
> >>> I understand it is important to check the return value in order to
> >>> have a consistent result when computing the remaining time but that
> >>> should not trigger a warning each time.
> >>
> >> Yes so WARN sounds appropriate. It should never happen. I don't see
> >> the issue.
> >
> > The issue is if there is an userspace application reading the ioctl
> > and or the sysfs, then the warning will be emitted each time if the
> > never- happen condition exists. Preferably replace the WARN_ON by
> > pr_warn_once() as suggested if the bug must be reported.
> 
> Sounds a bit funny 'if the never-happen condition exists' :-)
> 
> However, I will be fine with WARN_ON_ONCE(). I think that this warrants
> more of a large WARN splat than pr_warn() because it should never happen.
> 

I believe the two WARN_ON I used here are the "this-should-never-happen" situations. In my opinion, we are good to keep. But I am also good with WARN_ON_ONCE for compromise. I wonder if Daniel is good with this then I can fix them.

> Jon
> 
> --
> nvpublic

RE: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Robert Lin]

> -----Original Message-----
> From: Jon Hunter <jonathanh@nvidia.com>
> Sent: Friday, May 2, 2025 6:16 PM
> To: Daniel Lezcano <daniel.lezcano@linaro.org>; Robert Lin
> <robelin@nvidia.com>
> Cc: thierry.reding@gmail.com; tglx@linutronix.de; Pohsun Su
> <pohsuns@nvidia.com>; linux-kernel@vger.kernel.org; linux-
> tegra@vger.kernel.org; Sumit Gupta <sumitg@nvidia.com>
> Subject: Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add
> WDIOC_GETTIMELEFT support
> 
> 
> 
> On 02/05/2025 10:19, Daniel Lezcano wrote:
> > On Fri, May 02, 2025 at 12:37:25PM +0800, Robert Lin wrote:
> >> From: Pohsun Su <pohsuns@nvidia.com>
> >>
> >> 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>
> >> Signed-off-by: Robert Lin <robelin@nvidia.com>
> >> ---
> >>   drivers/clocksource/timer-tegra186.c | 64
> +++++++++++++++++++++++++++-
> >>   1 file changed, 63 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clocksource/timer-tegra186.c
> >> b/drivers/clocksource/timer-tegra186.c
> >> index ea742889ee06..8d5698770cbd 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-2025 NVIDIA Corporation. All rights reserved.
> >>    */
> >>
> >> +#include <linux/bitfield.h>
> >>   #include <linux/clocksource.h>
> >>   #include <linux/module.h>
> >>   #include <linux/interrupt.h>
> >> @@ -30,6 +31,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)
> >> @@ -46,6 +48,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) @@ -235,12 +240,69 @@
> 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 expiration, val;
> >> +	u64 timeleft;
> >> +
> >> +	if (!watchdog_active(&wdt->base)) {
> >> +		/* return zero if the watchdog timer is not activated. */
> >> +		return 0;
> >> +	}
> >> +
> >> +	/*
> >> +	 * 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
> >> +	 */
> >> +	val = readl_relaxed(wdt->regs + WDTSR);
> >> +	expiration = FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, val);
> >> +	if (WARN_ON(expiration > 4))
> >> +		return 0;
> >
> > Each call will generate a big warning in the message. May be simpler
> > to add a pr_err() with an explicit message.
> 
> I prefer the WARN. This should never happen. If we do change this, then
> dev_WARN() might be more appropriate, but I think that this is fine too.
> 
> >
> >> +	/* Get the current counter value in microsecond. */
> >> +	val = readl_relaxed(wdt->tmr->regs + TMRSR);
> >> +	timeleft = FIELD_GET(TMRSR_PCV, val);
> >> +
> >> +	/*
> >> +	 * Calculate the time remaining by adding the time for the
> >> +	 * counter value to the time of the counter expirations that
> >> +	 * remain.
> >> +	 */
> >> +	timeleft += (((u64)wdt->base.timeout * USEC_PER_SEC) / 5) * (4 -
> >> +expiration);
> >> +
> >> +	/*
> >> +	 * Convert the current counter value to seconds,
> >> +	 * rounding up to the nearest second. Cast u64 to
> >> +	 * u32 under the assumption that no overflow happens
> >> +	 * when coverting to seconds.
> >> +	 */
> >> +	timeleft = (timeleft + USEC_PER_SEC / 2) / USEC_PER_SEC;
> >
> > Did you check there is a macro fitting the need in math.h ?
> 
> I had a quick look, but looking again, I see we can use
> DIV_ROUND_CLOSEST_ULL() here.

I'll use the macro to refactor. Thanks.

> 
> >
> >> +	if (WARN_ON(timeleft > U32_MAX))
> >
> > s/WARN_ON/pr_err/
> 
> Why? Again this is not expected. I think that this is fine.
> 
> >
> >> +		return U32_MAX;
> >> +
> >> +	return lower_32_bits(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.34.1
> >>
> >
> 
> --
> nvpublic

Re: [PATCH v7 1/3] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support

[Daniel Lezcano]

On 05/05/2025 07:34, Robert Lin wrote:

[ ... ]

>>>> Yes so WARN sounds appropriate. It should never happen. I
>>>> don't see the issue.
>>> 
>>> The issue is if there is an userspace application reading the
>>> ioctl and or the sysfs, then the warning will be emitted each
>>> time if the never- happen condition exists. Preferably replace
>>> the WARN_ON by pr_warn_once() as suggested if the bug must be
>>> reported.
>> 
>> Sounds a bit funny 'if the never-happen condition exists' :-)
>> 
>> However, I will be fine with WARN_ON_ONCE(). I think that this
>> warrants more of a large WARN splat than pr_warn() because it
>> should never happen.
>> 
> 
> I believe the two WARN_ON I used here are the "this-should-never-
> happen" situations. In my opinion, we are good to keep. But I am
> also good with WARN_ON_ONCE for compromise. I wonder if Daniel is
> good with this then I can fix them.

I'm fine with this


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog