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-jon...@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
