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"). The Get_maintainers.pl file didn't work for me I got... ../../../scripts/get_maintainer.pl 0001-USB-serial-ch341-fix-wrong-baud-rate-setting-calcula.patch ../../../scripts/get_maintainer.pl: The current directory does not appear to be a linux kernel source tree. 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 really 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 could be a better solution than special cases for each affected baud rate. Something like this seems to work and what I used for testing... // chip frequency is 12Mhz. not quite the same as (CH341_BAUDBASE_FACTOR>>7) #define CH341_OSC_FREQUENCY 12000000 // //this block of code is for deciding when LSB==7 of "a" is beter than LSB=={0,1,2,3} of "a". //frequencies such as 921600, 256000, 1333333 etc have a large baud rate error when using LSB=={0,1,2,3} //such as the above code uses. 921600 when using LSB=={0,1,2,3} I measured 7.4% error but when //using LSB==7 I measured 0.4% error. // //The chip I tested this on was a CH340G and came back with "Chip version: 0x31". //I assume this will work for CH341 and other chip versions but I don't think I //have any of those chips lying around. // //calc baud error using the 0,1,2,3 LSB and also the error without the divisor (LSB==7) uint32_t msB=(a>>8)&0xFF; uint32_t lsB=a&0xFF; int32_t baud_wanted=(priv->baud_rate); uint32_t denom=((1<<(10-3*lsB))*(256-msB)); if(denom&&((baud_wanted+100)>=(((uint32_t)CH341_OSC_FREQUENCY)/256)))//as baud_wanted==(CH341_OSC_FREQUENCY/256) implies MSB==0 for no divisor, the 100 is for rounding. { //calc error for divisor int32_t baud_expected=((uint32_t)CH341_OSC_FREQUENCY)/denom; uint32_t baud_error_difference=abs(baud_expected-baud_wanted); //calc a for no divisor uint32_t a_no_divisor=(((256<<8)-(((uint32_t)CH341_OSC_FREQUENCY)<<8)/baud_wanted+128 )&0xFF00)|0x07; //a_no_divisor is only valid for MSB<248. if((a_no_divisor>>8)<248) { //calc error for no divisor int32_t baud_expected_no_divisor=((uint32_t)CH341_OSC_FREQUENCY)/(256-(a_no_divisor> >8)); uint32_t baud_error_difference_no_divisor=abs(baud_expected_no_divisor-baud_wanted); //if error using no divisor is less than using a divisor then use it instead for the "a" word. if(baud_error_difference_no_divisor<baud_error_difference) { a=a_no_divisor; } printk("ch341_set_baudrate_lcr: using LSB==%u. org_expected_baud=%u org_error=%u org_a=0x%4X no_divisor_expected_baud=%u no_divisor_error=%u no_divisor_a=0x%4X\n",a&0xFF,baud_expected,baud_error_difference,(msB<<8|lsB ),baud_expected_no_divisor,baud_error_difference_no_divisor,a_no_divisor); } } I've tested that 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... 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. The driver even with my patch doesn't give an error to the user when the error is beyond a certain threshold. With the formulas I deduced it would be possible to estimate the percentage error and if it was beyond a certain threshold send something to dmesg. For example the ch34x can't produce 1843200 baud with an acceptable accuracy but the driver will happily calculate the best possible "a". The user could read dmesg and adjust their hardware to better match the ch34x baud rate. Cheers, Jonti -----Original Message----- From: Johan Hovold [mailto:johan@xxxxxxxxxx] Sent: Monday, 3 June 2019 7:24 p.m. To: Jonathan Olds Cc: johan@xxxxxxxxxx; frank@xxxxxxxxxxxxxxxxxxxxxxxxxx; werner@xxxxxxxxxxxxxxxxxxxx; boris@xxxxxxxxxx Subject: Re: linux/drivers/usb/serial/ch341.c calculates some baud rates wrong Hi Jonathan, and sorry about the late reply. On Mon, May 20, 2019 at 10:37:48AM +1200, Jonathan Olds wrote: > Hi, > > Sorry for sending this email to so many but I've never contributed to > the Linux kernel before and I'm not sure how this is done or who to talk to. You can use scripts/get_maintainer.pl to figure out which maintainers and lists to CC. For USB serial that would be me and we always keep the linux-usb list on CC. It never hurts to CC the original authors as well, just like you did here. Note that you need to send mails to list as plain text, not html. > With a CH340 chip the ch341 driver is loaded. My chip id is 0x31 > according to the driver debug info. > > In "static int ch341_set_baudrate_lcr(struct usb_device *dev, struct > ch341_private *priv, u8 lcr)" > > > Currently we have... ... > a = (factor & 0xff00) | divisor; > This "a" thing is called index when used in this function... > > "ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, a);" > > This "a" is wrong for some baud rates. (I've only tried 921600 so far) > > For example at 921600 baud "a" is calculated as "0xF303". This creates > a baud rate of about 857000 baud. This can be seen in the following figures... > I saw this is another driver... ... > So this person used cases for various baud rates that have incorrect > rates using the method that the current Linux kernel driver implements. > > I then use the 0xF307 that this calculates for 921600 baud and put an > if statement into the current Linux driver like so... ... > This produced the correct baud rate as can be seen in the following > figure... > I found this bug out of necessity. I made a board ( > http://jontio.zapto.org/jpic ) and programmed initially in Windows > without any baud rate issues. Then moving to Linux the board wasn't > able to connect as the baud rate where incorrect. So it is a real bug > and one that needs addressing. Fortunately the solution looks like > using the code with the cases in it. I have no idea where the > programming documentation is for this chip so this index number for > setting the baud rate is just a magic number to me at the moment. > > What is the process to get this fix incorporated into new updates to > Linux kernel? If you want I can create a list of what standard baud > rates are incorrect and by how much with an oscilloscope; I think it > will only be for the very high baud rates and may only affect 921600 and above. Such a list would be very useful to have. And I agree that if you can't infer the algorithm used from your experimental data we may need to start special casing. Please do gather the data, and send it to the list and propose a patch for fixing the broken rates. Thanks, Johan
