Skip to contentSkip to conversation

ColdFire: Fix UART baudrate at 115200

PATCH New Tsi-Chung.Liew 3 participants 7 messages May 28, 2008 – May 29, 2008

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ColdFire: Fix UART baudrate at 115200

From: TsiChung Liew <[email protected]>

If bus frequency is larger than 133MHz, the UART cannot output baudrate at 115200 correctly.

---
 drivers/serial/mcfuart.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/serial/mcfuart.c +4 -1
index 88f3eb1..fca76bd 100644
@@ -64,7 +64,10 @@ int serial_init(void)
 
 	/* Setting up BaudRate */
 	counter = (u32) (gd->bus_clk / (gd->baudrate));
-	counter >>= 5;
+	counter = (counter + 31) >> 5;
+
+	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
+		counter++;
 
 	/* write to CTUR: divide counter upper byte */
 	uart->ubg1 = (u8) ((counter & 0xff00) >> 8);
Signature · 1 line
1.5.4.1
#1 lore raw

PATCH 1/1 ColdFire: Fix UART baudrate at 115200

Quoted 16 lines · Tsi-Chung.Liew

From: TsiChung Liew <[email protected]>

If bus frequency is larger than 133MHz, the UART cannot output baudrate at 115200 correctly.

---
 drivers/serial/mcfuart.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/serial/mcfuart.c +0 -0
index 88f3eb1..fca76bd 100644
@@ -64,7 +64,10 @@ int serial_init(void)
 

drivers/serial/mcfuart.c int serial_init(void) · line 73

Quoted 10 lines
drivers/serial/mcfuart.c +4 -1
 	/* Setting up BaudRate */
 	counter = (u32) (gd->bus_clk / (gd->baudrate));
-	counter >>= 5;
+	counter = (counter + 31) >> 5;
+
+	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
+		counter++;
 
 	/* write to CTUR: divide counter upper byte */
 	uart->ubg1 = (u8) ((counter & 0xff00) >> 8);

This doesn't look right at all. It looks like you are patching up integer math problems by using different bad math and then special casing the result. (In the metal working world, this is known as "measure with a micrometer, mark with chalk, cut with a torch, and grind to fit.")

Part A:
-------

Quoted 1 line

counter = (u32) (gd->bus_clk / (gd->baudrate));

If you want this to be more accurate, you should round it:

Quoted 1 line

counter = (u32) ((gd->bus_clk + (gd->baudrate / 2)) / (gd->baudrate));

Part B:
-------

drivers/serial/mcfuart.c int serial_init(void) · line 67

Quoted 1 line
drivers/serial/mcfuart.c +1 -0
+	counter = (counter + 31) >> 5;

This is not rounding properly, it is doing a "ceiling".

drivers/serial/mcfuart.c

Quoted 1 line
drivers/serial/mcfuart.c +1 -0
+	counter = (counter + 16) >> 5;

Part C:
-------

drivers/serial/mcfuart.c int serial_init(void) · line 70

Quoted 2 lines
drivers/serial/mcfuart.c +2 -0
+	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
+		counter++;

This looks totally bogus. I very strongly suspect you need this because the above parts A: and B: are not rounding the division math properly, resulting in a wrong divisor *for your configuration*. While the above conditional may result in the correct divisor *for your configuration,* it probably will be wrong for some finite set of *other* configurations.

If I got my algebra correct and my parenthesis balanced, I believe the above is trying to calculate the following formula:

	counter = (u32) (
		((gd->bus_clk + (gd->baudrate / 2) +
		 (gd->baudrate * 16))
		/ (gd->baudrate * 32);

Note, however, that (gd->baudrate / 2) is going to be relatively small compared to gd->bus_clk and (gd->baudrate * 16), so I suspect that the above formula could be changed to the following formula with no significant impact in accuracy:

	counter = (u32) (
		((gd->bus_clk + (gd->baudrate * 16))
		/ (gd->baudrate * 32);

Hmmmm, checking the math with 133e6 for the bus rate with full precision math (i.e. a calculator), I get a divisor value of 36.08 (truncated to an integer => 36). Your hack-math results in a divisor of 38. The last formula I proposed above result in a divisor of 36.58 (truncated to an integer => 36). That checks.

Real math => 36.08
Hack-math => 38 (109375 baud => 5.1% off)
Last formula => 36 (115451 baud => 0.2% off)

For 160e6 bus frequency:
Real math => 43.40
Hack-math => 44 (113636 baud => 1.4% off)
Last formula => 43 (116279 baud => 0.1% off)

I don't see how your formula works (well, actually it works but only because async serial can handle ~10% error). In fact, it looks to me that your "correction" actually results in *more* error than the original calculation *without* rounding. What is your actual bus speed?
What are to using on the other end of the serial line - I'm wondering
if your receiver end is substantially off, causing a stack-up error that is causing your problem??? Is your board hardware marginal at 115200 baud - what does it look like on a scope???

Best regards,
gvb

#2 lore raw

PATCH 1/1 ColdFire: Fix UART baudrate at 115200

Jerry,

The calculation you provided does not work, I tried it before.
The best approach is reverse calculation after finding the closer counter (or divider), then find the smaller gap to the bus frequency.

Using the calculation without the fix: counter = ((bus_freq / baudrate)
+ 31 )/ 32
Bus freq 140Mhz and baudrate 115200

counter = ((140000000 / 115200) + 31) / 32
counter = 38

However, the 38 divider shows partial garbage message when output to the terminal.

Now, perform reverse calculation to obtain bus frequency. bus_freq = ((counter * 32) - 31) * 115200

bus_freq = ((38 * 32) - 31) * 115200
bus_freq = 136512000
diff = 140000000 - 136512000 = 3488000

counter bus_freq diff
======= ========= =======
36 129139200 way too off!
37 132825600 7174400
38 136512000 3488000
39 140198400 198400 <=== Work at this counter! Small
diff.
40 143884800 3884800

Regards,
TsiChung

-----Original Message-----
From: Jerry Van Baren [mailto:[email protected]]
Sent: Wednesday, May 28, 2008 1:45 PM
To: Liew Tsi Chung
Cc: U-Boot-Users; Wolfgang Denx; Rigby John
Subject: Re: [U-Boot-Users] [PATCH] ColdFire: Fix UART baudrate at 115200

Quoted 17 lines · Tsi-Chung.Liew

From: TsiChung Liew <[email protected]>

If bus frequency is larger than 133MHz, the UART cannot output baudrate at 115200 correctly.

---
 drivers/serial/mcfuart.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/serial/mcfuart.c index renamed from drivers/serial/mcfuart.c +0 -0

88f3eb1..fca76bd 100644

drivers/serial/mcfuart.c +0 -0
@@ -64,7 +64,10 @@ int serial_init(void)
 

drivers/serial/mcfuart.c int serial_init(void) · line 73

Quoted 10 lines
drivers/serial/mcfuart.c +4 -1
 	/* Setting up BaudRate */
 	counter = (u32) (gd->bus_clk / (gd->baudrate));
-	counter >>= 5;
+	counter = (counter + 31) >> 5;
+
+	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
+		counter++;
 
 	/* write to CTUR: divide counter upper byte */
 	uart->ubg1 = (u8) ((counter & 0xff00) >> 8);

This doesn't look right at all. It looks like you are patching up integer math problems by using different bad math and then special casing the result. (In the metal working world, this is known as "measure with a micrometer, mark with chalk, cut with a torch, and grind to fit.")

Part A:
-------

Quoted 1 line

counter = (u32) (gd->bus_clk / (gd->baudrate));

If you want this to be more accurate, you should round it:

Quoted 1 line

counter = (u32) ((gd->bus_clk + (gd->baudrate / 2)) /

(gd->baudrate));

Part B:
-------

drivers/serial/mcfuart.c int serial_init(void) · line 67

Quoted 1 line
drivers/serial/mcfuart.c +1 -0
+	counter = (counter + 31) >> 5;

This is not rounding properly, it is doing a "ceiling".

drivers/serial/mcfuart.c

Quoted 1 line
drivers/serial/mcfuart.c +1 -0
+	counter = (counter + 16) >> 5;

Part C:
-------

drivers/serial/mcfuart.c int serial_init(void) · line 70

Quoted 2 lines
drivers/serial/mcfuart.c +2 -0
+	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
+		counter++;

This looks totally bogus. I very strongly suspect you need this because the above parts A: and B: are not rounding the division math properly, resulting in a wrong divisor *for your configuration*. While the above conditional may result in the correct divisor *for your configuration,* it probably will be wrong for some finite set of *other* configurations.

If I got my algebra correct and my parenthesis balanced, I believe the above is trying to calculate the following formula:

	counter = (u32) (
		((gd->bus_clk + (gd->baudrate / 2) +
		 (gd->baudrate * 16))
		/ (gd->baudrate * 32);

Note, however, that (gd->baudrate / 2) is going to be relatively small compared to gd->bus_clk and (gd->baudrate * 16), so I suspect that the above formula could be changed to the following formula with no significant impact in accuracy:

	counter = (u32) (
		((gd->bus_clk + (gd->baudrate * 16))
		/ (gd->baudrate * 32);

Hmmmm, checking the math with 133e6 for the bus rate with full precision math (i.e. a calculator), I get a divisor value of 36.08 (truncated to an integer => 36). Your hack-math results in a divisor of 38. The last formula I proposed above result in a divisor of 36.58 (truncated to an integer => 36). That checks.

Real math => 36.08
Hack-math => 38 (109375 baud => 5.1% off)
Last formula => 36 (115451 baud => 0.2% off)

For 160e6 bus frequency:
Real math => 43.40
Hack-math => 44 (113636 baud => 1.4% off)
Last formula => 43 (116279 baud => 0.1% off)

I don't see how your formula works (well, actually it works but only because async serial can handle ~10% error). In fact, it looks to me that your "correction" actually results in *more* error than the original calculation *without* rounding. What is your actual bus speed?

What are to using on the other end of the serial line - I'm wondering
if your receiver end is substantially off, causing a stack-up error that is causing your problem??? Is your board hardware marginal at 115200 baud - what does it look like on a scope???

Best regards,
gvb

#3 lore raw

PATCH 1/1 ColdFire: Fix UART baudrate at 115200

Quoted 8 lines · Liew Tsi Chung

Jerry,

The calculation you provided does not work, I tried it before.
The best approach is reverse calculation after finding the closer counter (or divider), then find the smaller gap to the bus frequency.

Using the calculation without the fix: counter = ((bus_freq / baudrate)
+ 31 )/ 32

Where are you getting the +31 from? Is this in the User's Manual?

Quoted 10 lines

Bus freq 140Mhz and baudrate 115200

counter = ((140000000 / 115200) + 31) / 32
counter = 38

However, the 38 divider shows partial garbage message when output to the terminal.

Now, perform reverse calculation to obtain bus frequency. bus_freq = ((counter * 32) - 31) * 115200

Computations are for mathematicians. Real engineers measure the actual frequency. ;-)

If you change the " - 31" to " - 42", your computations will result in a different divisor. Why are you subtracting what appears to be an arbitrary number (31)?

Quoted 13 lines

bus_freq = ((38 * 32) - 31) * 115200
bus_freq = 136512000
diff = 140000000 - 136512000 = 3488000

counter bus_freq diff
======= ========= =======
36 129139200 way too off!
37 132825600 7174400
38 136512000 3488000
39 140198400 198400 <=== Work@this counter! Small
diff.
40 143884800 3884800

Quoted 2 lines

Regards,
TsiChung

Are you sure your input frequency is 140MHz and not 144MHZ??? Have you read the itty-bitty numbers on the crystal? Have you put a scope on it and actually measured it? Never trust those hardware people. ;-)

Looking at the MFC5407 (arbitrarily, not knowing what processor you have), the dividers are as expected and *do not* match your "- 31" calculations. They have a simple example and simple math that matches (no "- 31").

Hmmm, the MFC5407 has a max CLKIN (and thus bus clock) of 54MHz. You must have a different flavor. MFC548x maxes at 50MHz. What processor are you using? Ahh, MFC5249 has a 140MHz bus. Still is a straight forward divide-by n*32, no "- 31".

gvb

Quoted 115 lines

-----Original Message-----
From: Jerry Van Baren [mailto:gerald.vanbaren at ge.com]
Sent: Wednesday, May 28, 2008 1:45 PM
To: Liew Tsi Chung
Cc: U-Boot-Users; Wolfgang Denx; Rigby John
Subject: Re: [U-Boot-Users] [PATCH] ColdFire: Fix UART baudrate at 115200

Quoted 17 lines · Tsi-Chung.Liew

From: TsiChung Liew <[email protected]>

If bus frequency is larger than 133MHz, the UART cannot output baudrate at 115200 correctly.

---
 drivers/serial/mcfuart.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/serial/mcfuart.c index renamed from drivers/serial/mcfuart.c +0 -0

88f3eb1..fca76bd 100644

drivers/serial/mcfuart.c +0 -0
@@ -64,7 +64,10 @@ int serial_init(void)
 

drivers/serial/mcfuart.c int serial_init(void) · line 73

Quoted 10 lines
drivers/serial/mcfuart.c +4 -1
 	/* Setting up BaudRate */
 	counter = (u32) (gd->bus_clk / (gd->baudrate));
-	counter >>= 5;
+	counter = (counter + 31) >> 5;
+
+	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
+		counter++;
 
 	/* write to CTUR: divide counter upper byte */
 	uart->ubg1 = (u8) ((counter & 0xff00) >> 8);

This doesn't look right at all. It looks like you are patching up integer math problems by using different bad math and then special casing the result. (In the metal working world, this is known as "measure with a micrometer, mark with chalk, cut with a torch, and grind to fit.")

Part A:
-------

Quoted 1 line

counter = (u32) (gd->bus_clk / (gd->baudrate));

If you want this to be more accurate, you should round it:

Quoted 1 line

counter = (u32) ((gd->bus_clk + (gd->baudrate / 2)) /

(gd->baudrate));

Part B:
-------

drivers/serial/mcfuart.c int serial_init(void) · line 67

Quoted 1 line
drivers/serial/mcfuart.c +1 -0
+	counter = (counter + 31) >> 5;

This is not rounding properly, it is doing a "ceiling".

drivers/serial/mcfuart.c

Quoted 1 line
drivers/serial/mcfuart.c +1 -0
+	counter = (counter + 16) >> 5;

Part C:
-------

drivers/serial/mcfuart.c int serial_init(void) · line 70

Quoted 2 lines
drivers/serial/mcfuart.c +2 -0
+	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
+		counter++;

This looks totally bogus. I very strongly suspect you need this because the above parts A: and B: are not rounding the division math properly, resulting in a wrong divisor *for your configuration*. While the above conditional may result in the correct divisor *for your configuration,* it probably will be wrong for some finite set of *other* configurations.

If I got my algebra correct and my parenthesis balanced, I believe the above is trying to calculate the following formula:

	counter = (u32) (
		((gd->bus_clk + (gd->baudrate / 2) +
		 (gd->baudrate * 16))
		/ (gd->baudrate * 32);

Note, however, that (gd->baudrate / 2) is going to be relatively small compared to gd->bus_clk and (gd->baudrate * 16), so I suspect that the above formula could be changed to the following formula with no significant impact in accuracy:

	counter = (u32) (
		((gd->bus_clk + (gd->baudrate * 16))
		/ (gd->baudrate * 32);

Hmmmm, checking the math with 133e6 for the bus rate with full precision math (i.e. a calculator), I get a divisor value of 36.08 (truncated to an integer => 36). Your hack-math results in a divisor of 38. The last formula I proposed above result in a divisor of 36.58 (truncated to an integer => 36). That checks.

Real math => 36.08
Hack-math => 38 (109375 baud => 5.1% off)
Last formula => 36 (115451 baud => 0.2% off)

For 160e6 bus frequency:
Real math => 43.40
Hack-math => 44 (113636 baud => 1.4% off)
Last formula => 43 (116279 baud => 0.1% off)

I don't see how your formula works (well, actually it works but only because async serial can handle ~10% error). In fact, it looks to me that your "correction" actually results in *more* error than the original calculation *without* rounding. What is your actual bus speed?

What are to using on the other end of the serial line - I'm wondering
if your receiver end is substantially off, causing a stack-up error that is causing your problem??? Is your board hardware marginal at 115200 baud - what does it look like on a scope???

Best regards,
gvb

#4 lore raw

PATCH 1/1 ColdFire: Fix UART baudrate at 115200

Quoted 1 line

Where are you getting the +31 from? Is this in the User's Manual?

No in user's manual. The +31 is rounding up purpose.

Quoted 1 line

Computations are for mathematicians. Real engineers measure the

actual frequency. ;-)
Agree.

Quoted 1 line

If you change the " - 31" to " - 42", your computations will result in

a different divisor.

Quoted 1 line

Why are you subtracting what appears to be an arbitrary number (31)?

Can't use 42 when the user's manual spec the divisor is 32. The "- 31" comes from counter = ((bus_freq / baudrate) + 31) / 32. Using the same calculation as mentioned here to find out bus_freq. bus_freq = ((counter
* 32) - 31) * baudrate.

Quoted 1 line

Are you sure your input frequency is 140MHz and not 144MHZ??? Have

you read the itty-bitty

Quoted 1 line

numbers on the crystal? Have you put a scope on it and actually

measured it? Never trust

Quoted 1 line

those hardware people. ;-)

Actually, the bus freq is 70Mhz, 140Mhz is cpu freq (scope measurement). The input freq (crystal/oscillator) not always 1 to 1 ratio. Take M5253EVBE for example, the input clk is 11Mhz. Then, it multiply up and divide down for CPU and bus frequency.

Quoted 1 line

Looking at the MFC5407 (arbitrarily, not knowing what processor you

have), the dividers are

Quoted 2 lines

as expected and *do not* match your "- 31"
calculations. They have a simple example and simple math that matches

(no "- 31").

Quoted 1 line

Hmmm, the MFC5407 has a max CLKIN (and thus bus clock) of 54MHz. You

must have a different

Quoted 2 lines

flavor. MFC548x maxes at 50MHz.
What processor are you using?

52x2, 532x, 547x_8x, 5445x. The 5253EVB is having the issue where the calculation does not apply if baudrate is 115200.

Quoted 1 line

Ahh, MFC5249 has a 140MHz bus. Still is a straight forward divide-by

n*32, no "- 31".
I thought the same too, but the 140Mhz is cpu freq.

Regards,
TsiChung

#5 lore raw

PATCH 1/1 ColdFire: Fix UART baudrate at 115200

Quoted 2 lines · Liew Tsi Chung
Quoted 1 line

Where are you getting the +31 from? Is this in the User's Manual?

No in user's manual. The +31 is rounding up purpose.

The formula (n + 31) / 32 is *NOT ROUNDING* the answer, it is doing a "ceiling" function. You need to add 1/2 the divisor to *ROUND*, i.e. (n
+ 16) / 32.

Quoted 11 lines
Quoted 1 line

Computations are for mathematicians. Real engineers measure the

actual frequency. ;-)
Agree.

Quoted 1 line

If you change the " - 31" to " - 42", your computations will result in

a different divisor.

Quoted 1 line

Why are you subtracting what appears to be an arbitrary number (31)?

Can't use 42 when the user's manual spec the divisor is 32. The "- 31" comes from counter = ((bus_freq / baudrate) + 31) / 32. Using the same calculation as mentioned here to find out bus_freq. bus_freq = ((counter
* 32) - 31) * baudrate.

The 42 is an obscure reference to the Hitchhiker's Guide to the Galaxy - the answer to the question of life, the universe and everything. In other words, it is a totally arbitrary number. Your use of +31 is not a *ROUNDING*, it is *CEILING* and thus I claim it is arbitrary, especially with the +1 to the divisor if your baud rate is suppose to be 115200 baud. You don't understand your hardware (if it makes you feel better, neither do I, but it is important that *you* understand it).

Quoted 9 lines
Quoted 1 line

Are you sure your input frequency is 140MHz and not 144MHZ??? Have

you read the itty-bitty

Quoted 1 line

numbers on the crystal? Have you put a scope on it and actually

measured it? Never trust

Quoted 1 line

those hardware people. ;-)

Actually, the bus freq is 70Mhz, 140Mhz is cpu freq (scope measurement). The input freq (crystal/oscillator) not always 1 to 1 ratio. Take M5253EVBE for example, the input clk is 11Mhz. Then, it multiply up and divide down for CPU and bus frequency.

This *REALLY* makes your formula wrong. According to the MCF5253 Reference Manual Eqn. 15-1, the baud rate is
fsys / (32 * divisor)
and fsys is 70MHz, *NOT* 140MHz.

baud = SysClk / (32 * divisor)

divisor = (70e6 / 32) / baud

divisor = 2187500 / 115200

divisor = 18.99 => 19

To *round*, add 1/2 the desired baud rate

divisor = ((70e6 / 32) * (baud / 2)) / baud

divisor = (2187500 + 57600) / 115200

divisor = 19.5 => integer truncation => 19

----

This means gd->bus_clk is *70MHz* (1/2 the CPU frequency), *NOT* 140MHz as you assume (note that this matches the Reference Manual).

I return to my contention:

Note, however, that (gd->baudrate / 2) is going to be relatively small compared to gd->bus_clk and (gd->baudrate * 16), so I suspect that the above formula could be changed to the following formula with no significant impact in accuracy:

	counter = (u32) (
		((gd->bus_clk + (gd->baudrate * 16))
		/ (gd->baudrate * 32);

Simplifying the *32 factor to match my formula above:

	counter = (u32) (
		((gd->bus_clk / 32) + (gd->baudrate / 2))
		/ gd->baudrate);

	counter = ((70e6 / 32) + (115200 / 2)) / 115200
	counter = 19   (after inter truncation)

Note that, without adding (baudrate / 2) - rounding, the above formula results in a divisor of 18 rather than the proper 19. THIS IS YOUR PROBLEM and also what your fix should be.

If you rounded properly, the baud rate calculation would work without all the magic.

Here is what your change actually does:

/* Setting up BaudRate */

	counter = (u32) (gd->bus_clk / (gd->baudrate));

This divide is doing an integer truncation and you are losing significant digits.
70e6 / 115200 = 607.64 => integer truncation = 607

	counter = (counter + 31) >> 5;

This is "ceiling", but you've already lost the digits due to the previous divide with integer truncation (no rounding).
(607 + 31) / 32 = 19.9 => integer truncation = 19

You get lucky and get the right answer even though you don't deserve it. ;-)

	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
		counter++;

This is totally bogus. gd->bus_clk == 70e6 so this is *always false.*

Quoted 8 lines
Quoted 1 line

Looking at the MFC5407 (arbitrarily, not knowing what processor you

have), the dividers are

Quoted 2 lines

as expected and *do not* match your "- 31"
calculations. They have a simple example and simple math that matches

(no "- 31").

Quoted 1 line

Hmmm, the MFC5407 has a max CLKIN (and thus bus clock) of 54MHz. You

must have a different

Quoted 8 lines
Quoted 2 lines

flavor. MFC548x maxes at 50MHz.
What processor are you using?

52x2, 532x, 547x_8x, 5445x. The 5253EVB is having the issue where the calculation does not apply if baudrate is 115200.

Quoted 1 line

Ahh, MFC5249 has a 140MHz bus. Still is a straight forward divide-by

n*32, no "- 31".
I thought the same too, but the 140Mhz is cpu freq.

It is clocked by the System Clock, not CPU frequency. The CPU frequency is 2x the System Clock.

Quoted 2 lines

Regards,
TsiChung

HTH,
gvb

#6 lore raw

PATCH 1/1 ColdFire: Fix UART baudrate at 115200

I see your point. I use your formula if you have not other comments.

Regards,
TsiChung

-----Original Message-----
From: Jerry Van Baren [mailto:gvb.uboot at gmail.com]
Sent: Wednesday, May 28, 2008 10:57 PM
To: Liew Tsi Chung
Cc: Jerry Van Baren; U-Boot-Users; Wolfgang Denx; Rigby John
Subject: Re: [U-Boot-Users] [PATCH] ColdFire: Fix UART baudrate at 115200

Quoted 2 lines · Liew Tsi Chung
Quoted 1 line

Where are you getting the +31 from? Is this in the User's Manual?

No in user's manual. The +31 is rounding up purpose.

The formula (n + 31) / 32 is *NOT ROUNDING* the answer, it is doing a "ceiling" function. You need to add 1/2 the divisor to *ROUND*, i.e. (n

+ 16) / 32.

Quoted 5 lines
Quoted 1 line

Computations are for mathematicians. Real engineers measure the

actual frequency. ;-)
Agree.

Quoted 1 line

If you change the " - 31" to " - 42", your computations will result

in

Quoted 5 lines

a different divisor.

Quoted 1 line

Why are you subtracting what appears to be an arbitrary number (31)?

Can't use 42 when the user's manual spec the divisor is 32. The "- 31" comes from counter = ((bus_freq / baudrate) + 31) / 32. Using the same calculation as mentioned here to find out bus_freq. bus_freq =

((counter

Quoted 1 line

* 32) - 31) * baudrate.

The 42 is an obscure reference to the Hitchhiker's Guide to the Galaxy -

the answer to the question of life, the universe and everything. In other words, it is a totally arbitrary number. Your use of +31 is not a

*ROUNDING*, it is *CEILING* and thus I claim it is arbitrary, especially

with the +1 to the divisor if your baud rate is suppose to be 115200 baud. You don't understand your hardware (if it makes you feel better, neither do I, but it is important that *you* understand it).

Quoted 6 lines
Quoted 1 line

Are you sure your input frequency is 140MHz and not 144MHZ??? Have

you read the itty-bitty

Quoted 1 line

numbers on the crystal? Have you put a scope on it and actually

measured it? Never trust

Quoted 1 line

those hardware people. ;-)

Actually, the bus freq is 70Mhz, 140Mhz is cpu freq (scope

measurement).

Quoted 2 lines

The input freq (crystal/oscillator) not always 1 to 1 ratio. Take M5253EVBE for example, the input clk is 11Mhz. Then, it multiply up

and

Quoted 1 line

divide down for CPU and bus frequency.

This *REALLY* makes your formula wrong. According to the MCF5253 Reference Manual Eqn. 15-1, the baud rate is
fsys / (32 * divisor)
and fsys is 70MHz, *NOT* 140MHz.

baud = SysClk / (32 * divisor)

divisor = (70e6 / 32) / baud

divisor = 2187500 / 115200

divisor = 18.99 => 19

To *round*, add 1/2 the desired baud rate

divisor = ((70e6 / 32) * (baud / 2)) / baud

divisor = (2187500 + 57600) / 115200

divisor = 19.5 => integer truncation => 19

----

This means gd->bus_clk is *70MHz* (1/2 the CPU frequency), *NOT* 140MHz as you assume (note that this matches the Reference Manual).

I return to my contention:

Note, however, that (gd->baudrate / 2) is going to be relatively small compared to gd->bus_clk and (gd->baudrate * 16), so I suspect that the above formula could be changed to the following formula with no significant impact in accuracy:

	counter = (u32) (
		((gd->bus_clk + (gd->baudrate * 16))
		/ (gd->baudrate * 32);

Simplifying the *32 factor to match my formula above:

	counter = (u32) (
		((gd->bus_clk / 32) + (gd->baudrate / 2))
		/ gd->baudrate);

	counter = ((70e6 / 32) + (115200 / 2)) / 115200
	counter = 19   (after inter truncation)

Note that, without adding (baudrate / 2) - rounding, the above formula results in a divisor of 18 rather than the proper 19. THIS IS YOUR PROBLEM and also what your fix should be.

If you rounded properly, the baud rate calculation would work without all the magic.

Here is what your change actually does:

/* Setting up BaudRate */

	counter = (u32) (gd->bus_clk / (gd->baudrate));

This divide is doing an integer truncation and you are losing significant digits.
70e6 / 115200 = 607.64 => integer truncation = 607

	counter = (counter + 31) >> 5;

This is "ceiling", but you've already lost the digits due to the previous divide with integer truncation (no rounding).
(607 + 31) / 32 = 19.9 => integer truncation = 19

You get lucky and get the right answer even though you don't deserve it. ;-)

	if ((gd->bus_clk > 133333333) && (gd->baudrate >= 115200))
		counter++;

This is totally bogus. gd->bus_clk == 70e6 so this is *always false.*

Quoted 4 lines
Quoted 1 line

Looking at the MFC5407 (arbitrarily, not knowing what processor you

have), the dividers are

Quoted 2 lines

as expected and *do not* match your "- 31" calculations. They have a simple example and simple math that

matches

Quoted 4 lines

(no "- 31").

Quoted 1 line

Hmmm, the MFC5407 has a max CLKIN (and thus bus clock) of 54MHz. You

must have a different

Quoted 8 lines
Quoted 2 lines

flavor. MFC548x maxes at 50MHz.
What processor are you using?

52x2, 532x, 547x_8x, 5445x. The 5253EVB is having the issue where the calculation does not apply if baudrate is 115200.

Quoted 1 line

Ahh, MFC5249 has a 140MHz bus. Still is a straight forward divide-by

n*32, no "- 31".
I thought the same too, but the 140Mhz is cpu freq.

It is clocked by the System Clock, not CPU frequency. The CPU frequency

is 2x the System Clock.

Quoted 2 lines

Regards,
TsiChung

HTH,
gvb