Round the serial port clock divisor value returned by calc_divisor()

PATCH New Hugo Villeneuve 3 participants 9 messages Jul 8, 2008 – Jul 15, 2008

lore

Share & tools

lore

Hugo Villeneuve

author

2008-07-08 17:32 UTC #0 lore raw

PATCH 1/1 Round the serial port clock divisor value returned by calc_divisor()

Round the serial port clock divisor value returned by calc_divisor().

Signed-off-by Hugo Villeneuve <[email protected]>

---

Rounding is important, especially when using high baud rates values like 115200bps. When using the non-rounded value, some boards will work and some won't.

 drivers/serial/serial.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

drivers/serial/serial.c +6 -1

index 76425d8..7e315ad 100644
@@ -124,6 +124,8 @@ static NS16550_t serial_ports[4] = {
 
 static int calc_divisor (NS16550_t port)
 {
+	u32 divisor_x10;
+
 #ifdef CONFIG_OMAP1510
 	/* If can't cleanly clock 115200 set div to 1 */
 	if ((CFG_NS16550_CLK == 12000000) && (gd->baudrate == 115200)) {

@@ -144,8 +146,11 @@ static int calc_divisor (NS16550_t port)
 #else
 #define MODE_X_DIV 16
 #endif
-	return (CFG_NS16550_CLK / MODE_X_DIV / gd->baudrate);
 
+	/* Compute divisor value with rounding by adding 0.5 */
+	divisor_x10 = (10 * CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate;
+
+	return (divisor_x10 + 5) / 10;
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)

Will a real rounding work? Work better? If I got my mental math and parenthesis right and the resulting math doesn't overflow your registers, the following will add 1/2 the baud rate scaled by the MODE_X_DIV and then perform the divide which will do full rounding.

	return (((CFG_NS16550_CLK + ((gd->baudrate / 2)* MODE_X_DIV))
		/ MODE_X_DIV) / gd->baudrate);

Alternately, I prefer to scale up by 16 and then divide by 8 since processors can do that very efficiently ( << 4 followed by >> 3).

Best regards,
gvb

Hugo Villeneuve

author

2008-07-08 18:21 UTC #2 lore raw

PATCH 1/1 Round the serial port clock divisor value returned by calc_divisor()

Quoted 40 lines · Jerry Van Baren

Quoted 26 lines · Hugo Villeneuve
Round the serial port clock divisor value returned by calc_divisor().
Signed-off-by Hugo Villeneuve <[email protected]>
---
Rounding is important, especially when using high baud rates values like 115200bps. When using the non-rounded value, some boards will work and some won't.
 drivers/serial/serial.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
drivers/serial/serial.c index renamed from drivers/serial/serial.c +0 -0
76425d8..7e315ad 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c
drivers/serial/serial.c index renamed from drivers/serial/serial.c +2 -0
@@ -124,6 +124,8 @@ static NS16550_t serial_ports[4] = {
 
 static int calc_divisor (NS16550_t port)
 {
+	u32 divisor_x10;
+
 #ifdef CONFIG_OMAP1510
 	/* If can't cleanly clock 115200 set div to 1 */
 	if ((CFG_NS16550_CLK == 12000000) && (gd->baudrate == 115200)) {
drivers/serial/serial.c index static int calc_divisor (NS16550_t port) #else · line 154
Quoted 10 lines
drivers/serial/serial.c index renamed from drivers/serial/serial.c +3 -1
@@ -144,8 +146,11 @@ static int calc_divisor (NS16550_t port)  #else
 #define MODE_X_DIV 16
 #endif
-	return (CFG_NS16550_CLK / MODE_X_DIV / gd->baudrate);
 
+	/* Compute divisor value with rounding by adding 0.5 */
+	divisor_x10 = (10 * CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate; +
+	return (divisor_x10 + 5) / 10;
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)
Hi Hugo,

Quoted 7 lines

Will a real rounding work? Work better? If I got my mental math and parenthesis right and the resulting math doesn't overflow your registers, the following will add 1/2 the baud rate scaled by the MODE_X_DIV and then perform the divide which will do full rounding.
	return (((CFG_NS16550_CLK + ((gd->baudrate / 2)* MODE_X_DIV))
		/ MODE_X_DIV) / gd->baudrate);

I tested it and it works for me.
But maybe the code is less obvious that way?

Quoted 2 lines

Alternately, I prefer to scale up by 16 and then divide by 8 since processors can do that very efficiently ( << 4 followed by >> 3).

Right.

I prefer the second solution for its simplicity, but the first one also works.

Hugo.

Hugo Villeneuve
Hardware developer | Concepteur mat?riel
Lyrtech
Phone/T?l. : (1) (418) 877-4644 #2395
Toll-free/Sans frais - Canada & USA : (1) (888) 922-4644 #2395 Fax/T?l?c. : (1) (418) 877-7710
www.lyrtech.com
Infinite possibilities...TM

Jerry Van Baren

2008-07-08 18:35 UTC #3 lore raw

PATCH 1/1 Round the serial port clock divisor value returned by calc_divisor()

Quoted 40 lines · Hugo Villeneuve

Quoted 39 lines · Jerry Van Baren
Quoted 26 lines · Hugo Villeneuve
Round the serial port clock divisor value returned by calc_divisor().
Signed-off-by Hugo Villeneuve <[email protected]>
---
Rounding is important, especially when using high baud rates values like 115200bps. When using the non-rounded value, some boards will work and some won't.
 drivers/serial/serial.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
drivers/serial/serial.c index renamed from drivers/serial/serial.c +0 -0
76425d8..7e315ad 100644 --- a/drivers/serial/serial.c +++ b/drivers/serial/serial.c
drivers/serial/serial.c index renamed from drivers/serial/serial.c +2 -0
@@ -124,6 +124,8 @@ static NS16550_t serial_ports[4] = {
 
 static int calc_divisor (NS16550_t port)
 {
+	u32 divisor_x10;
+
 #ifdef CONFIG_OMAP1510
 	/* If can't cleanly clock 115200 set div to 1 */
 	if ((CFG_NS16550_CLK == 12000000) && (gd->baudrate == 115200)) {
drivers/serial/serial.c index static int calc_divisor (NS16550_t port) #else · line 154
Quoted 10 lines
drivers/serial/serial.c index renamed from drivers/serial/serial.c +3 -1
@@ -144,8 +146,11 @@ static int calc_divisor (NS16550_t port)  #else
 #define MODE_X_DIV 16
 #endif
-	return (CFG_NS16550_CLK / MODE_X_DIV / gd->baudrate);
 
+	/* Compute divisor value with rounding by adding 0.5 */
+	divisor_x10 = (10 * CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate; +
+	return (divisor_x10 + 5) / 10;
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)
Hi Hugo,

Quoted 6 lines

Quoted 6 lines
Will a real rounding work? Work better? If I got my mental math and parenthesis right and the resulting math doesn't overflow your registers, the following will add 1/2 the baud rate scaled by the MODE_X_DIV and then perform the divide which will do full rounding.
	return (((CFG_NS16550_CLK + ((gd->baudrate / 2)* MODE_X_DIV))

oops, I missed a space ^

Quoted 3 lines

Quoted 1 line
		/ MODE_X_DIV) / gd->baudrate);
I tested it and it works for me.
But maybe the code is less obvious that way?

Obviousness is relative. :-)
This will be more accurate, but accuracy is relative too. :-/ It only has to be close.

Since CFG_NS16550_CLK is very likely a multiple of MODE_X_DIV or it doesn't matter if it isn't because it is much, much larger than MODE_X_DIV, the formula could be simplified slightly without substantially hurting the accuracy...

	return (((CFG_NS16550_CLK + (gd->baudrate / 2))
		/ MODE_X_DIV) / gd->baudrate);

A slight bit of added perceived complexity may be coming from my addition of parenthesis to emphasize the evaluation order. The following (should be) equivalent and reads better to my brain.

	return (CFG_NS16550_CLK + (gd->baudrate / 2)) /
		(MODE_X_DIV * gd->baudrate);

Quoted 5 lines

Quoted 2 lines
Alternately, I prefer to scale up by 16 and then divide by 8 since processors can do that very efficiently ( << 4 followed by >> 3).
Right.
I prefer the second solution for its simplicity, but the first one also works.

Bottom line: it's your call IMHO.

Best regards,
gvb

Hugo Villeneuve

author

2008-07-08 18:54 UTC #4 lore raw

PATCH 1/1 Round the serial port clock divisor value returned by calc_divisor()

Quoted 72 lines · Jerry Van Baren

Quoted 40 lines · Hugo Villeneuve
Quoted 39 lines · Jerry Van Baren
Quoted 27 lines · Hugo Villeneuve
Round the serial port clock divisor value returned by calc_divisor().
Signed-off-by Hugo Villeneuve <[email protected]>
---
Rounding is important, especially when using high baud rates values like 115200bps. When using the non-rounded value, some boards will work and some won't.
 drivers/serial/serial.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
drivers/serial/serial.c +0 -0
index 76425d8..7e315ad 100644 --- a/drivers/serial/serial.c +++
b/drivers/serial/serial.c @@ -124,6 +124,8 @@ static NS16550_t serial_ports[4] = {
static int calc_divisor (NS16550_t port)
{
drivers/serial/serial.c +2 -0
+	u32 divisor_x10;
+
 #ifdef CONFIG_OMAP1510
 	/* If can't cleanly clock 115200 set div to 1 */
 	if ((CFG_NS16550_CLK == 12000000) && (gd->baudrate == 115200)) {
drivers/serial/serial.c static int calc_divisor (NS16550_t port) · line 153
Quoted 9 lines
drivers/serial/serial.c +2 -1
@@ -144,8 +146,11 @@ static int calc_divisor (NS16550_t port) 
 #else  #define MODE_X_DIV 16 #endif
-	return (CFG_NS16550_CLK / MODE_X_DIV / gd->baudrate);
 
+	/* Compute divisor value with rounding by adding 0.5 */
+	divisor_x10 = (10 * CFG_NS16550_CLK) / MODE_X_DIV /
 gd->baudrate; + +	return (divisor_x10 + 5) / 10;
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)
Hi Hugo,
Quoted 6 lines
Quoted 6 lines
Will a real rounding work? Work better? If I got my mental math and parenthesis right and the resulting math doesn't overflow your registers, the following will add 1/2 the baud rate scaled by the MODE_X_DIV and then perform the divide which will do full rounding.
	return (((CFG_NS16550_CLK + ((gd->baudrate / 2)* MODE_X_DIV))
oops, I missed a space ^
Quoted 3 lines
Quoted 1 line
		/ MODE_X_DIV) / gd->baudrate);
I tested it and it works for me.
But maybe the code is less obvious that way?
Obviousness is relative. :-)
This will be more accurate, but accuracy is relative too. :-/ It only has to be close.
Since CFG_NS16550_CLK is very likely a multiple of MODE_X_DIV or it doesn't matter if it isn't because it is much, much larger than MODE_X_DIV, the formula could be simplified slightly without substantially hurting the accuracy...
	return (((CFG_NS16550_CLK + (gd->baudrate / 2))
		/ MODE_X_DIV) / gd->baudrate);
A slight bit of added perceived complexity may be coming from my addition of parenthesis to emphasize the evaluation order. The following (should be) equivalent and reads better to my brain.
	return (CFG_NS16550_CLK + (gd->baudrate / 2)) /
		(MODE_X_DIV * gd->baudrate);

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Alternately, I prefer to scale up by 16 and then divide by 8 since processors can do that very efficiently ( << 4 followed by >> 3). Right.
I prefer the second solution for its simplicity, but the first one also works.
Bottom line: it's your call IMHO.

Ok then, let?s use the second solution, updated patch will follow.

Hugo V.

Hugo Villeneuve

author

2008-07-15 15:23 UTC #5 lore raw

PATCH 1/1 Round the serial port clock divisor value returned by calc_divisor()

In reply to Hugo Villeneuve

Round the serial port clock divisor value returned by calc_divisor()

Signed-off-by Hugo Villeneuve <[email protected]>

---

 drivers/serial/serial.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/serial/serial.c +3 -9

index 4ccaee2..8bbfcf9 100644
@@ -124,8 +124,6 @@ static NS16550_t serial_ports[4] = {
 
 static int calc_divisor (NS16550_t port)
 {
-	uint32_t clk_divisor;
-
 #ifdef CONFIG_OMAP1510
 	/* If can't cleanly clock 115200 set div to 1 */
 	if ((CFG_NS16550_CLK == 12000000) && (gd->baudrate == 115200)) {

@@ -149,15 +147,11 @@ static int calc_divisor (NS16550_t port)
 
 	/* Compute divisor value. Normally, we should simply return:
 	 *   CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate
-	 * but we need to round that value by adding 0.5 (2/4).
+	 * but we need to round that value by adding 0.5.
 	 * Rounding is especially important at high baud rates.
 	 */
-	clk_divisor = (((4 * CFG_NS16550_CLK) /
-			(MODE_X_DIV * gd->baudrate)) + 2) / 4;
-
-	debug("NS16550 clock divisor = %d\n", clk_divisor);
-
-	return clk_divisor;
+	return (CFG_NS16550_CLK + (gd->baudrate * (MODE_X_DIV / 2))) /
+		(MODE_X_DIV * gd->baudrate);
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)

Jerry Van Baren

2008-07-15 15:53 UTC #6 lore raw

PATCH 1/1 Round the serial port clock divisor value returned by calc_divisor()

Quoted 4 lines · Hugo Villeneuve

Round the serial port clock divisor value returned by calc_divisor()
Signed-off-by Hugo Villeneuve <[email protected]>

Acked-by Gerald Van Baren <[email protected]>

(Hopefully this stops Wolfgang's pain!)

Quoted 48 lines

---

 drivers/serial/serial.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/serial/serial.c +3 ~~-10~~

index 4ccaee2..8bbfcf9 100644
@@ -124,8 +124,6 @@ static NS16550_t serial_ports[4] = {
 
 static int calc_divisor (NS16550_t port)
 {
-	uint32_t clk_divisor;
-
 #ifdef CONFIG_OMAP1510
 	/* If can't cleanly clock 115200 set div to 1 */
 	if ((CFG_NS16550_CLK == 12000000) && (gd->baudrate == 115200)) {

@@ -149,15 +147,11 @@ static int calc_divisor (NS16550_t port)
 
 	/* Compute divisor value. Normally, we should simply return:
 	 *   CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate
-	 * but we need to round that value by adding 0.5 (2/4).
+	 * but we need to round that value by adding 0.5.
 	 * Rounding is especially important at high baud rates.
 	 */
-	clk_divisor = (((4 * CFG_NS16550_CLK) /
-			(MODE_X_DIV * gd->baudrate)) + 2) / 4;
-
-	debug("NS16550 clock divisor = %d\n", clk_divisor);
-
-	return clk_divisor;
+	return (CFG_NS16550_CLK + (gd->baudrate * (MODE_X_DIV / 2))) /
+		(MODE_X_DIV * gd->baudrate);
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)
 
-------------------------------------------------------------------------

This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users

Wolfgang Denk

2008-07-15 19:54 UTC #7 lore raw

PATCH 1/1 Round the serial port clock divisor value returned by calc_divisor()

Quoted 9 lines · In message <[email protected]> you wrote:

Quoted 4 lines · Hugo Villeneuve
Round the serial port clock divisor value returned by calc_divisor()
Signed-off-by Hugo Villeneuve <[email protected]>
Acked-by Gerald Van Baren <[email protected]>
(Hopefully this stops Wolfgang's pain!)

Thanks for bringing this to a good end.

Best regards,

Wolfgang Denk

Signature · 4 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You cannot propel yourself forward by patting yourself on the back.

Wolfgang Denk

Applied

2008-07-15 19:53 UTC #8 lore raw

PATCH 1/1 Round the serial port clock divisor value returned by calc_divisor()

In reply to Hugo Villeneuve

Quoted 9 lines · In message <[email protected]> you…

Round the serial port clock divisor value returned by calc_divisor()
Signed-off-by Hugo Villeneuve <[email protected]>
---
 drivers/serial/serial.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

Signature · 6 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Prof:        So the American government went to IBM to come up with a
             data encryption standard and they came up with ...
Student:     EBCDIC!