Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.

Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.

[Nikolaj Fogh]

I have experienced that the ftdi_sio driver gives less-than-optimal baud rates as the driver truncates instead of rounds to nearest during baud rate divisor calculation.

This patch improves on the baud rate generation. The generated baud rate corresponds to the optimal baud rate achievable with the chip. This is what the windows driver gives as well.

Signed-off-by: Nikolaj Fogh <nikolajfogh@gmail.com>
---
 drivers/usb/serial/ftdi_sio.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 609198d9594c..0edbd3427548 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1130,7 +1130,7 @@ static unsigned short int ftdi_232am_baud_base_to_divisor(int baud, int base)
 {
 	unsigned short int divisor;
 	/* divisor shifted 3 bits to the left */
-	int divisor3 = base / 2 / baud;
+	int divisor3 = DIV_ROUND_CLOSEST(base, 2 * baud);
 	if ((divisor3 & 0x7) == 7)
 		divisor3++; /* round x.7/8 up to x+1 */
 	divisor = divisor3 >> 3;
@@ -1156,7 +1156,7 @@ static u32 ftdi_232bm_baud_base_to_divisor(int baud, int base)
 	static const unsigned char divfrac[8] = { 0, 3, 2, 4, 1, 5, 6, 7 };
 	u32 divisor;
 	/* divisor shifted 3 bits to the left */
-	int divisor3 = base / 2 / baud;
+	int divisor3 = DIV_ROUND_CLOSEST(base, 2 * baud);
 	divisor = divisor3 >> 3;
 	divisor |= (u32)divfrac[divisor3 & 0x7] << 14;
 	/* Deal with special cases for highest baud rates. */
@@ -1179,7 +1179,7 @@ static u32 ftdi_2232h_baud_base_to_divisor(int baud, int base)
 	int divisor3;
 
 	/* hi-speed baud rate is 10-bit sampling instead of 16-bit */
-	divisor3 = base * 8 / (baud * 10);
+	divisor3 = DIV_ROUND_CLOSEST(base * 8, baud * 10);
 
 	divisor = divisor3 >> 3;
 	divisor |= (u32)divfrac[divisor3 & 0x7] << 14;

Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.

[Johan Hovold]

On Thu, Nov 15, 2018 at 03:16:04PM +0100, Nikolaj Fogh wrote:
> On 11/15/18 9:24 AM, Johan Hovold wrote:
> > On Tue, Nov 13, 2018 at 08:19:44PM +0100, Nikolaj Fogh wrote:
> >> On 11/12/18 10:54 AM, Johan Hovold wrote:
> >>> On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote:
> >>>> I have experienced that the ftdi_sio driver gives less-than-optimal
> >>>> baud rates as the driver truncates instead of rounds to nearest
> >>>> during baud rate divisor calculation.
> >>>> This patch improves on the baud rate generation. The generated baud
> >>>> rate corresponds to the optimal baud rate achievable with the chip.
> >>>> This is what the windows driver gives as well.
> >>> How did you verify this? Did you trace and compare the divisors
> >>> actually requested by the Windows driver, or did you measure the
> >>> resulting rates using a scope?
> >> I verified it by scope. Granted, I only verified it for one baud rate
> >> (961200). Whether it gives the same as the Windows driver in general,
> >> I'm not sure. However, I would think that rounding instead of flooring
> >> would always yield the most accurate result.
> > I'm not so sure in this case. The driver uses "sub-integer" divisors and
> > looks like it depends on truncation rather than rounding. Some
> > background here:
> >
> > 	https://www.ftdichip.com/Support/Knowledgebase/index.html?whatbaudratesareachieveabl.htm

> I have had a closer look at this (and the driver code), and it seemsthat
> each bit in the divisorcorresponds to 1/8th (0.125) in the calculation.
> 
> It is shuffled around a bit in the code (for legacy reasons I expect), and
> put in the higher order bits, but prior to that, I see no reason that
> rounding should not be used instead of truncating. I don't see how it
> "depends" on truncation.

As I mentioned in my follow-up mail, I agree with that; your proposed
change looks correct.

> > If you want to change these calculations you need to make a stronger
> > case for it and verify that we don't mess up some other rate
> > inadvertently.

> I have done a calculation which compares the error of the baud rate
> calculation going all the way from 1 to 3MBaud where it can be seen that
> the rounding (as expected) halves the maximum error. Whereas the old method
> went up to 12% baud rate error, the new method reaches 6%, so the range
> of baud rates where communication will be successful should increase. Also,
> the new method is always better or as-accurate as the old.

Excellent, thanks for confirming. Just mention something about that in
the commit message too.

> I guess that image attachments are not welcome in the mailing list, so
> I will refrain from attaching it. Let me know if I should send it to
> you.

Sure, if you want too that'd be great.

> I am using it in a system which uses a baud rate of 961200 (and not
> the standard 921600). Here the old calculation gave an error of 4.03%
> and the new gave 0.12% error.

Also good to have in the commit message.

> I will try to verify the numbers I have calculated with a logic analyzer to
> make sure that it corresponds with the real world. I can also try to compare
> it to the windows driver outputs.

That would be really good, at least for a few rates.

> As I only have a FT232RT (232bm) to test with, the patch should probably be
> limited to the changes in the ftdi_232bm_baud_base_to_divisor() function.

No, as long as you mention which device you used for testing, and the
numbers for the other other types looks similar, I think we can go ahead
and round those divisions too.

Thanks for doing this!

Johan

Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.

[Nikolaj Fogh]

On 11/15/18 9:24 AM, Johan Hovold wrote:
> On Tue, Nov 13, 2018 at 08:19:44PM +0100, Nikolaj Fogh wrote:
>> On 11/12/18 10:54 AM, Johan Hovold wrote:
>>> On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote:
>>>> I have experienced that the ftdi_sio driver gives less-than-optimal
>>>> baud rates as the driver truncates instead of rounds to nearest
>>>> during baud rate divisor calculation.
>>>> This patch improves on the baud rate generation. The generated baud
>>>> rate corresponds to the optimal baud rate achievable with the chip.
>>>> This is what the windows driver gives as well.
>>> How did you verify this? Did you trace and compare the divisors
>>> actually requested by the Windows driver, or did you measure the
>>> resulting rates using a scope?
>> I verified it by scope. Granted, I only verified it for one baud rate
>> (961200). Whether it gives the same as the Windows driver in general,
>> I'm not sure. However, I would think that rounding instead of flooring
>> would always yield the most accurate result.
> I'm not so sure in this case. The driver uses "sub-integer" divisors and
> looks like it depends on truncation rather than rounding. Some
> background here:
>
> 	https://www.ftdichip.com/Support/Knowledgebase/index.html?whatbaudratesareachieveabl.htm
I have had a closer look at this (and the driver code), and it seemsthat
each bit in the divisorcorresponds to 1/8th (0.125) in the calculation.

It is shuffled around a bit in the code (for legacy reasons I expect), and
put in the higher order bits, but prior to that, I see no reason that
rounding should not be used instead of truncating. I don't see how it
"depends" on truncation.

> If you want to change these calculations you need to make a stronger
> case for it and verify that we don't mess up some other rate
> inadvertently.
I have done a calculation which compares the error of the baud rate
calculation going all the way from 1 to 3MBaud where it can be seen that
the rounding (as expected) halves the maximum error. Whereas the old method
went up to 12% baud rate error, the new method reaches 6%, so the range
of baud rates where communication will be successful should increase. Also,
the new method is always better or as-accurate as the old.

I guess that image attachments are not welcome in the mailing list, so I 
will
refrain from attaching it. Let me know if I should send it to you.

I am using it in a system which uses a baud rate of 961200 (and not the 
standard
921600). Here the old calculation gave an error of 4.03% and the new gave
0.12% error.

I will try to verify the numbers I have calculated with a logic analyzer to
make sure that it corresponds with the real world. I can also try to compare
it to the windows driver outputs.

As I only have a FT232RT (232bm) to test with, the patch should probably be
limited to the changes in the ftdi_232bm_baud_base_to_divisor() function.

Best regards
   Nikolaj Fogh
>
> Thanks,
> Johan

Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.

[Johan Hovold]

On Thu, Nov 15, 2018 at 09:24:49AM +0100, Johan Hovold wrote:
> On Tue, Nov 13, 2018 at 08:19:44PM +0100, Nikolaj Fogh wrote:
> > On 11/12/18 10:54 AM, Johan Hovold wrote:
> > > On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote:
> > >> I have experienced that the ftdi_sio driver gives less-than-optimal
> > >> baud rates as the driver truncates instead of rounds to nearest
> > >> during baud rate divisor calculation.
> 
> > >> This patch improves on the baud rate generation. The generated baud
> > >> rate corresponds to the optimal baud rate achievable with the chip.
> > >> This is what the windows driver gives as well.
> > >
> > > How did you verify this? Did you trace and compare the divisors
> > > actually requested by the Windows driver, or did you measure the
> > > resulting rates using a scope?
> 
> > I verified it by scope. Granted, I only verified it for one baud rate
> > (961200). Whether it gives the same as the Windows driver in general,
> > I'm not sure. However, I would think that rounding instead of flooring
> > would always yield the most accurate result.
> 
> I'm not so sure in this case. The driver uses "sub-integer" divisors and
> looks like it depends on truncation rather than rounding. Some
> background here:
> 
> 	https://www.ftdichip.com/Support/Knowledgebase/index.html?whatbaudratesareachieveabl.htm

After taking a closer look at the driver, I think your patch may be
fine. It shouldn't change 921600 baud, though, but I see now that you
wrote *961200* above. Was that intentional?

> If you want to change these calculations you need to make a stronger
> case for it and verify that we don't mess up some other rate
> inadvertently.

This still stands though, you need a better description of why this
is needed and correct. Mention which device and rate that was off, and
by how much. Determining the theoretical difference may be interesting
too, while making sure we don't introduce larger errors for other rates.

Thanks,
Johan

Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.

[Johan Hovold]

On Tue, Nov 13, 2018 at 08:19:44PM +0100, Nikolaj Fogh wrote:
> On 11/12/18 10:54 AM, Johan Hovold wrote:
> > On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote:
> >> I have experienced that the ftdi_sio driver gives less-than-optimal
> >> baud rates as the driver truncates instead of rounds to nearest
> >> during baud rate divisor calculation.

> >> This patch improves on the baud rate generation. The generated baud
> >> rate corresponds to the optimal baud rate achievable with the chip.
> >> This is what the windows driver gives as well.
> >
> > How did you verify this? Did you trace and compare the divisors
> > actually requested by the Windows driver, or did you measure the
> > resulting rates using a scope?

> I verified it by scope. Granted, I only verified it for one baud rate
> (961200). Whether it gives the same as the Windows driver in general,
> I'm not sure. However, I would think that rounding instead of flooring
> would always yield the most accurate result.

I'm not so sure in this case. The driver uses "sub-integer" divisors and
looks like it depends on truncation rather than rounding. Some
background here:

	https://www.ftdichip.com/Support/Knowledgebase/index.html?whatbaudratesareachieveabl.htm

If you want to change these calculations you need to make a stronger
case for it and verify that we don't mess up some other rate
inadvertently.

Thanks,
Johan

Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.

[Nikolaj Fogh]

On 11/12/18 10:54 AM, Johan Hovold wrote:
> On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote:
>> I have experienced that the ftdi_sio driver gives less-than-optimal baud rates as the driver truncates instead of rounds to nearest during baud rate divisor calculation.
> Please break your lines at 72 cols or so, and use the common subject
> prefix (e.g. "USB: serial: ftdi_sio: improve...") with a concise
> summary.
Ok. I will do that in the future.
>> This patch improves on the baud rate generation. The generated baud rate corresponds to the optimal baud rate achievable with the chip. This is what the windows driver gives as well.
> How did you verify this? Did you trace and compare the divisors
> actually requested by the Windows driver, or did you measure the
> resulting rates using a scope?
>
> Thanks,
> Johan
I verified it by scope. Granted, I only verified it for one baud rate
(961200). Whether it gives the same as the Windows driver in general,
I'm not sure. However, I would think that rounding instead of flooring
would always yield the most accurate result.

Best regards
   Nikolaj Fogh

Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.

[Johan Hovold]

On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote:
> I have experienced that the ftdi_sio driver gives less-than-optimal baud rates as the driver truncates instead of rounds to nearest during baud rate divisor calculation.

Please break your lines at 72 cols or so, and use the common subject
prefix (e.g. "USB: serial: ftdi_sio: improve...") with a concise
summary.

> This patch improves on the baud rate generation. The generated baud rate corresponds to the optimal baud rate achievable with the chip. This is what the windows driver gives as well.

How did you verify this? Did you trace and compare the divisors
actually requested by the Windows driver, or did you measure the
resulting rates using a scope?

Thanks,
Johan