All of lore.kernel.org
 help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
To: Wim Van Sebroeck <wim@iguana.be>, Guenter Roeck <linux@roeck-us.net>
Cc: Leela Krishna Amudala <l.krishna@samsung.com>,
	Olof Johansson <olof@lixom.net>,
	Tomasz Figa <tomasz.figa@gmail.com>,
	Kukjin Kim <kgene.kim@samsung.com>,
	Doug Anderson <dianders@chromium.org>,
	Ben Dooks <ben-linux@fluff.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org,
	linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH] watchdog: s3c2410_wdt: Handle rounding a little better for timeout
Date: Tue, 26 Nov 2013 10:30:37 -0800	[thread overview]
Message-ID: <1385490637-10306-1-git-send-email-dianders@chromium.org> (raw)

The existing watchdog timeout worked OK but didn't deal with
rounding in an ideal way when dividing out all of its clocks.

Specifically if you had a timeout of 32 seconds and an input clock of
66666666, you'd end up setting a timeout of 31.9998 seconds and
reporting a timeout of 31 seconds.

Specifically DBG printouts showed:
  s3c2410wdt_set_heartbeat: count=16666656, timeout=32, freq=520833
  s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=16666656 (0000ff4f)
and the final timeout reported to the user was:
  ((count / divisor) * divisor) / freq
  (0xff4f * 255) / 520833 = 31 (truncated from 31.9998)
the technically "correct" value is:
  (0xff4f * 255) / (66666666.0 / 128) = 31.9998

By using "DIV_ROUND_UP" we can be a little more correct.
  s3c2410wdt_set_heartbeat: count=16666688, timeout=32, freq=520834
  s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=16666688 (0000ff50)
and the final timeout reported to the user:
  (0xff50 * 255) / 520834 = 32
the technically "correct" value is:
  (0xff50 * 255) / (66666666.0 / 128) = 32.0003

We'll use a DIV_ROUND_UP to solve this, generally erroring on the side
of reporting shorter values to the user and setting the watchdog to
slightly longer than requested:
* Round input frequency up to assume watchdog is counting faster.
* Round divisions by divisor up to give us extra time.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/watchdog/s3c2410_wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7d8fd04..fe2322b 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -188,7 +188,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeou
 	if (timeout < 1)
 		return -EINVAL;
 
-	freq /= 128;
+	freq = DIV_ROUND_UP(freq, 128);
 	count = timeout * freq;
 
 	DBG("%s: count=%d, timeout=%d, freq=%lu\n",
@@ -201,20 +201,20 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeou
 
 	if (count >= 0x10000) {
 		for (divisor = 1; divisor <= 0x100; divisor++) {
-			if ((count / divisor) < 0x10000)
+			if (DIV_ROUND_UP(count, divisor) < 0x10000)
 				break;
 		}
 
-		if ((count / divisor) >= 0x10000) {
+		if (divisor > 0x100) {
 			dev_err(wdt->dev, "timeout %d too big\n", timeout);
 			return -EINVAL;
 		}
 	}
 
 	DBG("%s: timeout=%d, divisor=%d, count=%d (%08x)\n",
-	    __func__, timeout, divisor, count, count/divisor);
+	    __func__, timeout, divisor, count, DIV_ROUND_UP(count, divisor));
 
-	count /= divisor;
+	count = DIV_ROUND_UP(count, divisor);
 	wdt->count = count;
 
 	/* update the pre-scaler */
-- 
1.8.4.1


WARNING: multiple messages have this Message-ID (diff)
From: Doug Anderson <dianders@chromium.org>
To: Wim Van Sebroeck <wim@iguana.be>, Guenter Roeck <linux@roeck-us.net>
Cc: Kukjin Kim <kgene.kim@samsung.com>,
	linux-watchdog@vger.kernel.org,
	Leela Krishna Amudala <l.krishna@samsung.com>,
	linux-kernel@vger.kernel.org, Tomasz Figa <tomasz.figa@gmail.com>,
	Doug Anderson <dianders@chromium.org>,
	linux-samsung-soc@vger.kernel.org,
	Ben Dooks <ben-linux@fluff.org>, Olof Johansson <olof@lixom.net>,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH] watchdog: s3c2410_wdt: Handle rounding a little better for timeout
Date: Tue, 26 Nov 2013 10:30:37 -0800	[thread overview]
Message-ID: <1385490637-10306-1-git-send-email-dianders@chromium.org> (raw)

The existing watchdog timeout worked OK but didn't deal with
rounding in an ideal way when dividing out all of its clocks.

Specifically if you had a timeout of 32 seconds and an input clock of
66666666, you'd end up setting a timeout of 31.9998 seconds and
reporting a timeout of 31 seconds.

Specifically DBG printouts showed:
  s3c2410wdt_set_heartbeat: count=16666656, timeout=32, freq=520833
  s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=16666656 (0000ff4f)
and the final timeout reported to the user was:
  ((count / divisor) * divisor) / freq
  (0xff4f * 255) / 520833 = 31 (truncated from 31.9998)
the technically "correct" value is:
  (0xff4f * 255) / (66666666.0 / 128) = 31.9998

By using "DIV_ROUND_UP" we can be a little more correct.
  s3c2410wdt_set_heartbeat: count=16666688, timeout=32, freq=520834
  s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=16666688 (0000ff50)
and the final timeout reported to the user:
  (0xff50 * 255) / 520834 = 32
the technically "correct" value is:
  (0xff50 * 255) / (66666666.0 / 128) = 32.0003

We'll use a DIV_ROUND_UP to solve this, generally erroring on the side
of reporting shorter values to the user and setting the watchdog to
slightly longer than requested:
* Round input frequency up to assume watchdog is counting faster.
* Round divisions by divisor up to give us extra time.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/watchdog/s3c2410_wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7d8fd04..fe2322b 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -188,7 +188,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeou
 	if (timeout < 1)
 		return -EINVAL;
 
-	freq /= 128;
+	freq = DIV_ROUND_UP(freq, 128);
 	count = timeout * freq;
 
 	DBG("%s: count=%d, timeout=%d, freq=%lu\n",
@@ -201,20 +201,20 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeou
 
 	if (count >= 0x10000) {
 		for (divisor = 1; divisor <= 0x100; divisor++) {
-			if ((count / divisor) < 0x10000)
+			if (DIV_ROUND_UP(count, divisor) < 0x10000)
 				break;
 		}
 
-		if ((count / divisor) >= 0x10000) {
+		if (divisor > 0x100) {
 			dev_err(wdt->dev, "timeout %d too big\n", timeout);
 			return -EINVAL;
 		}
 	}
 
 	DBG("%s: timeout=%d, divisor=%d, count=%d (%08x)\n",
-	    __func__, timeout, divisor, count, count/divisor);
+	    __func__, timeout, divisor, count, DIV_ROUND_UP(count, divisor));
 
-	count /= divisor;
+	count = DIV_ROUND_UP(count, divisor);
 	wdt->count = count;
 
 	/* update the pre-scaler */
-- 
1.8.4.1

WARNING: multiple messages have this Message-ID (diff)
From: dianders@chromium.org (Doug Anderson)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] watchdog: s3c2410_wdt: Handle rounding a little better for timeout
Date: Tue, 26 Nov 2013 10:30:37 -0800	[thread overview]
Message-ID: <1385490637-10306-1-git-send-email-dianders@chromium.org> (raw)

The existing watchdog timeout worked OK but didn't deal with
rounding in an ideal way when dividing out all of its clocks.

Specifically if you had a timeout of 32 seconds and an input clock of
66666666, you'd end up setting a timeout of 31.9998 seconds and
reporting a timeout of 31 seconds.

Specifically DBG printouts showed:
  s3c2410wdt_set_heartbeat: count=16666656, timeout=32, freq=520833
  s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=16666656 (0000ff4f)
and the final timeout reported to the user was:
  ((count / divisor) * divisor) / freq
  (0xff4f * 255) / 520833 = 31 (truncated from 31.9998)
the technically "correct" value is:
  (0xff4f * 255) / (66666666.0 / 128) = 31.9998

By using "DIV_ROUND_UP" we can be a little more correct.
  s3c2410wdt_set_heartbeat: count=16666688, timeout=32, freq=520834
  s3c2410wdt_set_heartbeat: timeout=32, divisor=255, count=16666688 (0000ff50)
and the final timeout reported to the user:
  (0xff50 * 255) / 520834 = 32
the technically "correct" value is:
  (0xff50 * 255) / (66666666.0 / 128) = 32.0003

We'll use a DIV_ROUND_UP to solve this, generally erroring on the side
of reporting shorter values to the user and setting the watchdog to
slightly longer than requested:
* Round input frequency up to assume watchdog is counting faster.
* Round divisions by divisor up to give us extra time.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/watchdog/s3c2410_wdt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 7d8fd04..fe2322b 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -188,7 +188,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeou
 	if (timeout < 1)
 		return -EINVAL;
 
-	freq /= 128;
+	freq = DIV_ROUND_UP(freq, 128);
 	count = timeout * freq;
 
 	DBG("%s: count=%d, timeout=%d, freq=%lu\n",
@@ -201,20 +201,20 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeou
 
 	if (count >= 0x10000) {
 		for (divisor = 1; divisor <= 0x100; divisor++) {
-			if ((count / divisor) < 0x10000)
+			if (DIV_ROUND_UP(count, divisor) < 0x10000)
 				break;
 		}
 
-		if ((count / divisor) >= 0x10000) {
+		if (divisor > 0x100) {
 			dev_err(wdt->dev, "timeout %d too big\n", timeout);
 			return -EINVAL;
 		}
 	}
 
 	DBG("%s: timeout=%d, divisor=%d, count=%d (%08x)\n",
-	    __func__, timeout, divisor, count, count/divisor);
+	    __func__, timeout, divisor, count, DIV_ROUND_UP(count, divisor));
 
-	count /= divisor;
+	count = DIV_ROUND_UP(count, divisor);
 	wdt->count = count;
 
 	/* update the pre-scaler */
-- 
1.8.4.1

             reply	other threads:[~2013-11-26 18:35 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-11-26 18:30 Doug Anderson [this message]
2013-11-26 18:30 ` [PATCH] watchdog: s3c2410_wdt: Handle rounding a little better for timeout Doug Anderson
2013-11-26 18:30 ` Doug Anderson
2013-11-26 18:48 ` Guenter Roeck
2013-11-26 18:48   ` Guenter Roeck
2013-11-26 21:34   ` Doug Anderson
2013-11-26 21:34     ` Doug Anderson
2013-11-26 21:34     ` Doug Anderson
2013-11-27  0:10     ` Guenter Roeck
2013-11-27  0:10       ` Guenter Roeck
2013-11-27  0:10       ` Guenter Roeck
2013-11-27  0:57       ` Doug Anderson
2013-11-27  0:57         ` Doug Anderson
2013-11-27  0:57         ` Doug Anderson
2013-11-27  0:57         ` Doug Anderson
2013-11-27  0:57 ` [PATCH v2] " Doug Anderson
2013-11-27  0:57   ` Doug Anderson
2013-11-27  1:04   ` Guenter Roeck
2013-11-27  1:04     ` Guenter Roeck
2014-01-08 20:03   ` Wim Van Sebroeck

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=1385490637-10306-1-git-send-email-dianders@chromium.org \
    --to=dianders@chromium.org \
    --cc=ben-linux@fluff.org \
    --cc=kgene.kim@samsung.com \
    --cc=l.krishna@samsung.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-samsung-soc@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=olof@lixom.net \
    --cc=tomasz.figa@gmail.com \
    --cc=wim@iguana.be \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.