| Message ID | 20181031200913.GA6372@nfogh-Ryzen (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors. | expand |
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
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
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
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
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
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
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;
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(-)