[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <af0e0ab4-89d5-4f77-86c7-90d2bc3e13dd@nvidia.com>
Date: Fri, 2 May 2025 11:16:17 +0100
From: Jon Hunter <jonathanh@...dia.com>
To: Daniel Lezcano <daniel.lezcano@...aro.org>,
Robert Lin <robelin@...dia.com>
Cc: thierry.reding@...il.com, tglx@...utronix.de, pohsuns@...dia.com,
linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org, sumitg@...dia.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@...dia.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@...dia.com>
>> Signed-off-by: Robert Lin <robelin@...dia.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
Powered by blists - more mailing lists