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

[U-Boot-Users] [PATCH V2] 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>
Signed-off-by: John Roberts <john.roberts@pwav.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 |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 76425d8..1192f07 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -144,8 +144,12 @@ 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. Normally, we should simply return:
+	 *   CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate
+	 * 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;
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)

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

[Wolfgang Denk]

In message <1215543298-26921-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>
> Signed-off-by: John Roberts <john.roberts@pwav.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 |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)

Applied (afterfixing the broken multi-line comment). 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
What is mind?  No matter.  What is matter?  Never mind.
                                      -- Thomas Hewitt Key, 1799-1875

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

[Wolfgang Denk]

In message <1215543298-26921-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>
> Signed-off-by: John Roberts <john.roberts@pwav.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 |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
> index 76425d8..1192f07 100644
> --- a/drivers/serial/serial.c
> +++ b/drivers/serial/serial.c
> @@ -144,8 +144,12 @@ 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. Normally, we should simply return:
> +	 *   CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate
> +	 * 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;
>  }

Your patch causes problems with some boards:

Configuring for SBC8540 board...
serial.c: In function 'calc_divisor':
serial.c:153: warning: integer overflow in expression
serial.c:153: warning: integer overflow in expression

Configuring for sbc8548 board...
serial.c: In function 'calc_divisor':
serial.c:153: warning: integer overflow in expression
serial.c:153: warning: integer overflow in expression


Do you have a quick fix, or shall I back out the patch?

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
To understand a program you must become  both  the  machine  and  the
program.

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

[Jerry Van Baren]

Wolfgang Denk wrote:
> In message <1215543298-26921-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>
>> Signed-off-by: John Roberts <john.roberts@pwav.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 |    6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
>> index 76425d8..1192f07 100644
>> --- a/drivers/serial/serial.c
>> +++ b/drivers/serial/serial.c
>> @@ -144,8 +144,12 @@ 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. Normally, we should simply return:
>> +	 *   CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate
>> +	 * 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;
>>  }
> 
> Your patch causes problems with some boards:
> 
> Configuring for SBC8540 board...
> serial.c: In function 'calc_divisor':
> serial.c:153: warning: integer overflow in expression
> serial.c:153: warning: integer overflow in expression
> 
> Configuring for sbc8548 board...
> serial.c: In function 'calc_divisor':
> serial.c:153: warning: integer overflow in expression
> serial.c:153: warning: integer overflow in expression
> 
> 
> Do you have a quick fix, or shall I back out the patch?
> 
> Best regards,
> 
> Wolfgang Denk

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>

My apologies for providing the patch as an attachment.  I need to add 
the external editor plugin so I can use tbird for proper inline patches.

Best regards,
gvb

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Round-the-serial-port-clock-divisor-value-returned-b.patch
Type: text/x-patch
Size: 991 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080710/89e0d89a/attachment.bin 

[U-Boot-Users] [PATCH] Fix integer overflow warning in calc_divisor()

[Hugo Villeneuve]

Fix the integer overflow warning when rounding the serial
port clock divisor value in calc_divisor().

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

---

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

diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 182ca2d..4ccaee2 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)
 {
+	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)) {
@@ -147,10 +149,15 @@ 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 or 8/16.
+	 * but we need to round that value by adding 0.5 (2/4).
 	 * Rounding is especially important at high baud rates.
 	 */
-	return (((16 * CFG_NS16550_CLK) / MODE_X_DIV / gd->baudrate) + 8) / 16;
+	clk_divisor = (((4 * CFG_NS16550_CLK) /
+			(MODE_X_DIV * gd->baudrate)) + 2) / 4;
+
+	debug("NS16550 clock divisor = %d\n", clk_divisor);
+
+	return clk_divisor;
 }
 
 #if !defined(CONFIG_SERIAL_MULTI)

[U-Boot-Users] [PATCH] Fix integer overflow warning in calc_divisor()

[Wolfgang Denk]

In message <1215786255-4241-1-git-send-email-hugo.villeneuve@lyrtech.com> you wrote:
> Fix the integer overflow warning when rounding the serial
> port clock divisor value in calc_divisor().
> 
> Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>
> 
> ---
> 
>  drivers/serial/serial.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)

Applied, thanks, but I really wish you had kept the subject unchanged,
and ideally even threading correct.

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
In C we had to code our own bugs, in C++ we can inherit them.

[U-Boot-Users] [PATCH] Fix integer overflow warning in calc_divisor()

[Hugo Villeneuve]

On Sun, 2008-07-13 at 15:14 +0200, Wolfgang Denk wrote:
> In message <1215786255-4241-1-git-send-email-hugo.villeneuve@lyrtech.com> you wrote:
> > Fix the integer overflow warning when rounding the serial
> > port clock divisor value in calc_divisor().
> > 
> > Signed-off-by: Hugo Villeneuve <hugo.villeneuve@lyrtech.com>
> > 
> > ---
> > 
> >  drivers/serial/serial.c |   11 +++++++++--
> >  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> Applied, thanks, but I really wish you had kept the subject unchanged,
> and ideally even threading correct.

Hi Wolfgang,
since you already applied the first patch, I simply thought that the fix
for the first patch should have a different subject.

I will keep the same subject in the future

Hugo V.

[U-Boot-Users] [PATCH] Fix integer overflow warning in calc_divisor()

[Wolfgang Denk]

In message <1216047507.12227.2.camel@server.hugovil.com> you wrote:
> 
> since you already applied the first patch, I simply thought that the fix
> for the first patch should have a different subject.
> 
> I will keep the same subject in the future

Thanks. You have another try anyway ;-)

The bug is not fixed yet:

Configuring for mpc7448hpc2 board...
serial.c: In function 'calc_divisor':
serial.c:155: warning: integer overflow in expression
 

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
Anarchy may not be the best form of government, but it's better  than
no government at all.

[U-Boot-Users] [PATCH v2] 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>
---

This is my latest entry in the baud rate rounding dual.  Since it doesn't
multiply the master BRG clock but instead adds the baud rate scaled by
1/2 the clock multiplier, it should not overflow (for a master clock
right at the edge of overflowing itself, it still will overflow, but
that is pretty unlikely).

This compiles OK on the mpc7448hpc2.  I have only tested it on a
calculator.  I have NOT tested it on real hardware.

 drivers/serial/serial.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/serial/serial.c b/drivers/serial/serial.c
index 4ccaee2..7f43540 100644
--- a/drivers/serial/serial.c
+++ b/drivers/serial/serial.c
@@ -124,7 +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 */
@@ -147,17 +146,12 @@ static int calc_divisor (NS16550_t port)
 #define MODE_X_DIV 16
 #endif
 
-	/* 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).
-	 * Rounding is especially important at high baud rates.
+	/*
+	 * Compute divisor value, rounding it properly.  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)
-- 
1.5.6

[U-Boot-Users] [PATCH v2] 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.
> 
> Signed-off-by: Gerald Van Baren <vanbaren@cideas.com>
> ---
> 
> This is my latest entry in the baud rate rounding dual.  Since it
> doesn't multiply the master BRG clock but instead adds the baud rate
> scaled by 1/2 the clock multiplier, it should not overflow (for a
> master clock right at the edge of overflowing itself, it still will
> overflow, but that is pretty unlikely).
> 
> This compiles OK on the mpc7448hpc2.  I have only tested it on a
> calculator.  I have NOT tested it on real hardware.

I have tested it on my board and it works.

I also tested it on my board by using the clock value of the mpc7448hpc2 board, and printing the computed value which was correct.

>  drivers/serial/serial.c |   16 +++++-----------
>  1 files changed, 5 insertions(+), 11 deletions(-)
> ...
> -	/* 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).
> -	 * Rounding is especially important at high baud rates.
> +	/*
> +	 * Compute divisor value, rounding it properly.  Rounding is
> +	 * especially important at high baud rates.
>  	 */

Please keep the comments explaining the base formula: it is not easy to reverse-engineer an optimized formula to figure out what is the base formula.

I will resubmit a patch with your fix and my comments.

Hugo V.