[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <CZ96NM6U8O59.3TXG2WKAL7L8F@gmail.com>
Date: Mon, 19 Feb 2024 17:06:48 +0100
From: "Thierry Reding" <thierry.reding@...il.com>
To: "Pohsun Su" <pohsuns@...dia.com>, <daniel.lezcano@...aro.org>,
<tglx@...utronix.de>, <jonathanh@...dia.com>
Cc: <sumitg@...dia.com>, <linux-kernel@...r.kernel.org>,
<linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] clocksource/drivers/timer-tegra186: add
WDIOC_GETTIMELEFT support
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@...dia.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,
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists