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@xxxxxxxxxxxx/ > ). 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
