| Message ID | 20240216210258.24855-2-pohsuns@nvidia.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | clocksource: fix Tegra234 SoC Watchdog Timer. | expand |
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/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,
On Fri Feb 16, 2024 at 10:02 PM CET, Pohsun Su wrote: [...] > diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c [...] > @@ -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; One more thing I noticed: you could put both of these declarations on a single line to make this a bit more compact. Thierry
Hi Thierry, >> +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. I will change it from "system power-on reset" to "System POR(Power On Reset)" in next patch. AFAIK Power On Reset is used for decribing resetting circuits and initialing whatever it needs when received a POR signal after powered up. This term should also be applicable for hardware watchdog reset since the system is already powered up at that moment and >> + * 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. Sorry, I missed to check the line length before submit, will wrap comments in next patch. >> + >> + /* 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. Thanks for the suggestion, will do. >> + >> + /* >> + * 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? Here I made a transposition on purpose just for keeping the precision due to the integer division. e.g. If 'divided by 5' goes first, the equation goes to 0 when wdt->base.timeout is less than 5 no matter which timer expiration it is. But the number will be still inaccurate because I made a mistake that the number should be calculated in microseconds all the time and be converted to a nearest second only in the last step. A new patch has been made and I will submit it after verifications. Thank you for your time on reviewing this! Best, -- Pohsun
On Sat Feb 24, 2024 at 12:51 AM CET, Pohsun Su wrote: > Hi Thierry, > > >> +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. > > I will change it from "system power-on reset" to "System POR(Power On Reset)" in next patch. > AFAIK Power On Reset is used for decribing resetting circuits and initialing whatever it needs > when received a POR signal after powered up. This term should also be applicable for > hardware watchdog reset since the system is already powered up at that moment and "System POR" isn't an improvement over "system power-on reset". I'm mainly concerned that somebody might mistake this to somehow mean that there's an actual power cycle, which, as I understand, there isn't. So maybe just explain that this type of reset will put the system into the same state that it would be after a power cycle? Thierry
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 + * 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. */ + 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; + + /* + * 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; + 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,
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(-)