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

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