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

[Robert Lin]

> -----Original Message-----
> From: Jon Hunter <jonathanh@xxxxxxxxxx>
> Sent: Friday, May 2, 2025 6:16 PM
> To: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>; Robert Lin
> <robelin@xxxxxxxxxx>
> Cc: thierry.reding@xxxxxxxxx; tglx@xxxxxxxxxxxxx; Pohsun Su
> <pohsuns@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-
> tegra@xxxxxxxxxxxxxxx; Sumit Gupta <sumitg@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>
> >> Signed-off-by: Robert Lin <robelin@xxxxxxxxxx>
> >> ---
> >>   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