From: Pohsun Su <pohsuns@nvidia.com>
To: <thierry.reding@gmail.com>, <daniel.lezcano@linaro.org>,
<tglx@linutronix.de>, <jonathanh@nvidia.com>
Cc: <linux-kernel@vger.kernel.org>, <linux-tegra@vger.kernel.org>,
<sumitg@nvidia.com>, Pohsun Su <pohsuns@nvidia.com>
Subject: Re: [PATCH v2 1/2] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support
Date: Sat, 24 Feb 2024 07:51:26 +0800 [thread overview]
Message-ID: <20240223235126.31004-1-pohsuns@nvidia.com> (raw)
In-Reply-To: <CZ96NM6U8O59.3TXG2WKAL7L8F@gmail.com>
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
next prev parent reply other threads:[~2024-02-23 23:52 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-16 21:02 [PATCH v2 0/2] clocksource: fix Tegra234 SoC Watchdog Timer Pohsun Su
2024-02-16 21:02 ` [PATCH v2 1/2] clocksource/drivers/timer-tegra186: add WDIOC_GETTIMELEFT support Pohsun Su
2024-02-19 16:06 ` Thierry Reding
2024-02-23 23:51 ` Pohsun Su [this message]
2024-02-27 11:58 ` Thierry Reding
2024-02-19 16:18 ` Thierry Reding
2024-02-16 21:02 ` [PATCH v2 2/2] clocksource/drivers/timer-tegra186: fix watchdog self-pinging Pohsun Su
2024-02-19 16:17 ` Thierry Reding
2024-02-23 23:42 ` Pohsun Su
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240223235126.31004-1-pohsuns@nvidia.com \
--to=pohsuns@nvidia.com \
--cc=daniel.lezcano@linaro.org \
--cc=jonathanh@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=sumitg@nvidia.com \
--cc=tglx@linutronix.de \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).