[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Jerry Van Baren]

This formula is better at avoiding integer overflow.

Signed-off-by: Gerald Van Baren <vanbaren@cideas.com>
---

(try 2)

The formula I proposed previously in this thread compiles OK.  I cannot
verify it is correct on real hardware, however.
<http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/43499/focus=43506>

Best regards,
gvb 

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

diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 182ca2d..48579ae 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -150,7 +150,9 @@ static int calc_divisor (NS16550_t port)
 	 * but we need to round that value by adding 0.5 or 8/16.
 	 * Rounding is especially important at high baud rates.
 	 */
-	return (((16 * CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate) + 8) / 16;
+	return (CFG_NS16550_CLK + (gd->baudrate / 2)) /
+		(MODE_X_DIV * gd->baudrate);
+
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)
-- 
1.5.6

[U-Boot-Users] [PATCH] Round the serial port clock divisor valuereturned by calc_divisor()

[Hugo Villeneuve]

u-boot-users-bounces at lists.sourceforge.net wrote:
> This formula is better at avoiding integer overflow.

This patch doesn?t round the value correctly.

Wolfgang, I will resubmit a new patch shortly today to fix the problem.

Hugo V.

[U-Boot-Users] [PATCH] Round the serial port clock divisor valuereturned by calc_divisor()

[Jerry Van Baren]

Hugo Villeneuve wrote:
> u-boot-users-bounces at lists.sourceforge.net wrote:
>> This formula is better at avoiding integer overflow.
> 
> This patch doesn?t round the value correctly.
> 
> Wolfgang, I will resubmit a new patch shortly today to fix the problem.
> 
> Hugo V.

Arrgh, you are right.  I know you solved it already, but for the record 
I oversimplified:
   (gd->baudrate / 2)
should have been:
   (gd->baudrate * (MODE_X_DIV / 2))

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

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

Sorry,
gvb

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Hugo Villeneuve]

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

Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>

---

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(-)

diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
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)
 {
+	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)

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Jerry Van Baren]

Hugo Villeneuve wrote:
> Round the serial port clock divisor value returned by
> calc_divisor().
> 
> Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>
> 
> ---
> 
> 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(-)
> 
> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> 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)
>  {
> +	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)

Hi Hugo,

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

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Hugo Villeneuve]

Jerry Van Baren wrote:
> Hugo Villeneuve wrote:
>> Round the serial port clock divisor value returned by calc_divisor().
>> 
>> Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>
>> 
>> ---
>> 
>> 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(-)
>> 
>> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c 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)
>>  {
>> +	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)
> 
> Hi Hugo,
> 
> 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?
 
> 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

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Jerry Van Baren]

Hugo Villeneuve wrote:
> Jerry Van Baren wrote:
>> Hugo Villeneuve wrote:
>>> Round the serial port clock divisor value returned by calc_divisor().
>>>
>>> Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>
>>>
>>> ---
>>>
>>> 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(-)
>>>
>>> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c 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)
>>>  {
>>> +	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)
>> Hi Hugo,
>>
>> 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 ^
>> 		/ 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);


>> 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

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Hugo Villeneuve]

Jerry Van Baren wrote:
> Hugo Villeneuve wrote:
>> Jerry Van Baren wrote:
>>> Hugo Villeneuve wrote:
>>>> Round the serial port clock divisor value returned by
>>>> calc_divisor(). 
>>>> 
>>>> Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>
>>>> 
>>>> ---
>>>> 
>>>> 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(-)
>>>> 
>>>> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
>>>> 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)
>>>>  {
>>>> +	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)
>>> Hi Hugo,
>>> 
>>> 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 ^
>>> 		/ 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);
> 
> 
>>> 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
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

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Hugo Villeneuve]

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

Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>

---

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

diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 4ccaee2..8bbfcf9 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -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)

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Jerry Van Baren]

Hugo Villeneuve wrote:
> Round the serial port clock divisor value returned by
> calc_divisor()
> 
> Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>

Acked-by: Gerald Van Baren <vanbaren@cideas.com>

(Hopefully this stops Wolfgang's pain!)

> 
> ---
> 
>  drivers/serial/serial.c |   12 +++---------
>  1 files changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> index 4ccaee2..8bbfcf9 100644
> --- a/drivers/serial/serial.c
> +++ b/drivers/serial/serial.c
> @@ -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

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Wolfgang Denk]

In message <487CC7FF.2000700@ge.com> you wrote:
> Hugo Villeneuve wrote:
> > Round the serial port clock divisor value returned by
> > calc_divisor()
> > 
> > Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>
> 
> Acked-by: Gerald Van Baren <vanbaren@cideas.com>
> 
> (Hopefully this stops Wolfgang's pain!)

Thanks for bringing this to a good end.

Best regards,

Wolfgang Denk

-- 
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.

[U-Boot-Users] [PATCH] Round the serial port clock divisor value returned by calc_divisor()

[Wolfgang Denk]

In message <1216135382-21987-1-git-send-email-hugo.villeneuve@lyrtech.com> you wrote:
> Round the serial port clock divisor value returned by
> calc_divisor()
> 
> Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>
> 
> ---
> 
>  drivers/serial/serial.c |   12 +++---------
>  1 files changed, 3 insertions(+), 9 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
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!