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

[Jon Hunter <jonathanh@nvidia.com>]

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