Re: linux/drivers/usb/serial/ch341.c calculates some baud rates wrong

[Johan Hovold <johan@kernel.org>]

On Mon, Jun 10, 2019 at 02:32:16PM +1200, Jonathan Olds wrote:
> Hi Johan,
> 
> Thanks for the info. I followed
> https://nickdesaulniers.github.io/blog/2017/05/16/submitting-your-first-patc
> h-to-the-linux-kernel-and-responding-to-feedback/ and made a proposal patch
> ("[PATCH] USB: serial: ch341: fix wrong baud rate setting calculation"
> https://lore.kernel.org/linux-usb/20190608051309.4689-1-jontio@i4free.co.nz/
> ).

Good job, looks like you got most things right from the start, but I'll
comment on the patch directly.

> I've measured the actual baud rates for a lot of given baud rates and I
> think I have deduced the formulas for calculating the "a" value. "a" is a
> uint16 and split up in two, a LSB and a MSB. The current driver only uses
> LSB from the set {0,1,2,3}. There is another valid LSB of 7 that the current
> driver doesn't use. The formula for LSB from the set {0,1,2,3} is...
> 
> Actual baud rate == 2^(3*LSB-10)*12000000/(256-MSB), if LSB is in {0,1,2,3}
> and 0<MSB<255
> 
> When LSB == 7 then things are a bit different. LSB==7 seems to switch off
> the clock divider that the LSB usually does but only if MSB<248; when
> MSB>=248 the clock divider is turned back on and LSB is effectively 3 again.
> So we can also use the following formula...
> 
> Actual baud rate == 12000000/(256-MSB), if LSB == 7 and 0<MSB<248
> 
> So the trick is to use these formulas to find a MSB and a LSB that produce
> and actual baud rate that are as close as possible to the desired baud rate.
> With errors greater than say 2 to 3% hardware will start to fail to
> communicate.
> 
> Looking at some common baud rates only the higher rates are affected by not
> using a LSB of 7. Of the typical rates only 256000 and 921600 are affected.
> However other unusual frequencies are affected too such as 1333333 and I
> think you could calculate a lot more unusual affected frequencies. That
> being the case I think calculating the MSB when LSB == 7 for a given wanted
> baud rate might be a better solution than special cases for each affected
> baud rate.

Agreed.

> I've tested the patch with my hardware and it seems to work fine with every
> setting I could throw at it. I am aware that I've only tried it on my
> hardware with a CH340G chip. So trying with different chips and computers
> would be a good idea (I've tested it on the CH340G chip with two computers).
> 
> My measurements/workings as a libre/open office calc file can be downloaded
> from https://jontio.github.io/linux_kernel_work/ch43x_tests.ods .
> 
> I measured the following with the current driver (without my patch) using my
> scope...
> 
> Baud wanted	Baud measured	Error as % of wanted
> 50	50	0.0%
> 75	75.2	0.3%
> 110	109.5	0.5%
> 135	134.6	0.3%
> 150	150.4	0.3%
> 300	300.8	0.3%
> 600	601.3	0.2%
> 1200	1201.9	0.2%
> 1800	1801.8	0.1%
> 2400	2403.8	0.2%
> 4800	4807.7	0.2%
> 7200	7215	0.2%
> 9600	9615.4	0.2%
> 14400	14430	0.2%
> 19200	19231	0.2%
> 38400	38462	0.2%
> 56000	56054	0.1%
> 57600	57837	0.4%
> 115200	115207	0.0%
> 128000	127551	0.4%
> 230400	230415	0.0%
> 256000	250000	2.3%
> 460800	460617	0.0%
> 921600	853242	7.4%
> 1000000	999001	0.1%
> 1333333	1204819	9.6%
> 1843200	1496334	18.8%
> 2000000	1984127	0.8%
> 5000000	2985075	40.3%
> 
> The patch will fix 256000, 1333333 and 921600 but not 1843200 and 5000000.

Nice job indeed, I think some of the above belongs in the commit as well.

I don't have time to dig into the details myself at the moment, but I'll
point out some minor issues with you patch meanwhile.  

Thanks,
Johan