Received-SPF: pass (google.com: domain of linux-kernel+bounces-71639-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Content-Type: multipart/signed;
 boundary=b0da0a27ceabff049215f580b6f79f3cc323727d60ffb73d4a209fb6824b;
 micalg=pgp-sha256; protocol="application/pgp-signature"
Precedence: bulk
Mime-Version: 1.0
Date: Mon, 19 Feb 2024 17:06:48 +0100
Message-Id: <CZ96NM6U8O59.3TXG2WKAL7L8F@gmail.com>
Cc: <sumitg@nvidia.com>, <linux-kernel@vger.kernel.org>,
 <linux-tegra@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] clocksource/drivers/timer-tegra186: add
 WDIOC_GETTIMELEFT support
From: "Thierry Reding" <thierry.reding@gmail.com>
To: "Pohsun Su" <pohsuns@nvidia.com>, <daniel.lezcano@linaro.org>,
 <tglx@linutronix.de>, <jonathanh@nvidia.com>
References: <20240216210258.24855-1-pohsuns@nvidia.com>
 <20240216210258.24855-2-pohsuns@nvidia.com>
In-Reply-To: <20240216210258.24855-2-pohsuns@nvidia.com>

--b0da0a27ceabff049215f580b6f79f3cc323727d60ffb73d4a209fb6824b
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8

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@nvidia.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/t=
imer-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.
>   */
> =20
> +#include <linux/bitfield.h>
>  #include <linux/clocksource.h>
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
> @@ -29,6 +30,7 @@
> =20
>  #define TMRSR 0x004
>  #define  TMRSR_INTR_CLR BIT(30)
> +#define  TMRSR_PCV GENMASK(28, 0)
> =20
>  #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)
> =20
> +#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;
>  }
> =20
> +static unsigned int tegra186_wdt_get_timeleft(struct watchdog_device *wd=
d)
> +{
> +	struct tegra186_wdt *wdt =3D 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 i=
nto 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 par=
ts:
> +	 * 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 bet=
ween 0 and 4. */
> +	expiration =3D FIELD_GET(WDTSR_CURRENT_EXPIRATION_COUNT, readl_relaxed(=
wdt->regs + WDTSR));
> +
> +	/* Convert the current counter value to seconds, rounding up to the nea=
rest second. */
> +	timeleft =3D FIELD_GET(TMRSR_PCV, readl_relaxed(wdt->tmr->regs + TMRSR)=
);
> +	timeleft =3D (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 valu=
e
> +	 * to the time of the counter expirations that remain.
> +	 */
> +	timeleft +=3D wdt->base.timeout * (4 - expiration) / 5;

This doesn't quite match what the comment above says. Shouldn't this be:

	timeleft +=3D (wdt->base.timeout / 5) * (4 - expiration);

instead?

Thierry

> +	return timeleft;
> +}
> +
>  static const struct watchdog_ops tegra186_wdt_ops =3D {
>  	.owner =3D THIS_MODULE,
>  	.start =3D tegra186_wdt_start,
>  	.stop =3D tegra186_wdt_stop,
>  	.ping =3D tegra186_wdt_ping,
>  	.set_timeout =3D tegra186_wdt_set_timeout,
> +	.get_timeleft =3D tegra186_wdt_get_timeleft,
>  };
> =20
>  static struct tegra186_wdt *tegra186_wdt_create(struct tegra186_timer *t=
egra,


--b0da0a27ceabff049215f580b6f79f3cc323727d60ffb73d4a209fb6824b
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmXTfJgACgkQ3SOs138+
s6GORA//ZiipCoOSu/wox8hsJdL/TpRaNXjRNKNMz0NErbEYulBe30SnI7eT5XTW
lbTiLsc5jF+1u0PwmpYpOCpxH3iTWFbTR5GvSWE2DSjtVRkve+Laz6uYGetcip9w
MdN4t53myl86ehPL9eWMJesi20aqyfRbKkLP/rG424LItSgjnAqdRNSMlnBGozBY
2CFa3GGepna7+3VSFGhHcuvslCFc9xIyg4kGcdsiAStPRtnVrx0XEW8KSUFT+u+b
IjZi3w4EU3rYLCxvrrCQ8EcV0XMSQ7lmhcxFohsXK1cHx4hVtrTQ9RtXZcNQ3IZC
DxfwqBDa4eeCWmek+8Y4a8jLvE32gAzPyuVrfLwCckVkmvGDicMNrt91Dtu3UABU
aEsuKfM6DQJ0DXQeEqhwF3tfWSqbxIu9n0P20cBY65E/Pvu2yd+0VDW1VGUdGQba
64RHPWq/jvgQTOG+xX7S2gOjk/E/8Bnr2thKQ+eWsadU9ZMffOWFv+hyd+CfUnYk
IQ6V0ZIWGf6zmvjubb1XK+UVigr8bD1CETRUF7QOkXppdlbm2+P7pACgaewdaMyY
2nDFrpCCi8jextNbkKCsMk5tI/0kAsLJBhYg48Ki2gFl4s9L4FO8927CjxaBIJXd
oWJZDv766LuFtbYtvYCEXfBoqJvRWzq5v+dq5YVbl3Gp5qfJiM8=
=dCoG
-----END PGP SIGNATURE-----

--b0da0a27ceabff049215f580b6f79f3cc323727d60ffb73d4a209fb6824b--