Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Michael Dreher]

Hi Johan

>> https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/README.md
>Interesting! From a very quick glance this looks very promising (although he doesn't seem to handle rounding yet). I'll take a closer look and compare with what I've done. 

That’s my code. I actually do rounding. It might not be obvious that the '(*2 + 1) / 2’
in  ((2UL * CH341_OSC_F) + / (prescaler * priv->baud_rate) + 1UL) / 2UL
does a bankers rounding, that why I explained it in the README.MD:
https://github.com/nospam2000/ch341-baudrate-calculation#rounding-issues

Rounding issues

Why not just using the simple formula (CH341_CRYSTAL_FREQ / (prescaler * baud_rate))? Because we are using integer arithmetic and truncating values after the division leads to an error which only goes into the positive direction because the fractional part of divisor is lost.
With floating point arithmetic you would do 5/10 bankers rounding to spread the error equally between positive and negative range:

divisor = TRUNC(CH341_CRYSTAL_FREQ / (prescaler * baud_rate) + 0.5)

which is equal to

divisor = TRUNC((10 * CH341_CRYSTAL_FREQ / (prescaler * baud_rate) + 5) / 10)

The formula in the code does the same but using dual system integer arithmetic and therefore use *2 +1 /2 instead of *10 +5 /10.

> I also noticed that we should always pick the higher base clock if the resulting divisor is odd (and within the valid range).

The general rule from the perspective of mathematics is to use the highest possible divider (and therefore the lowest possible prescaler)
because that gives the lowest possible relative error in the division. As long as the result of divider*prescaler is the same it doesn’t
matter which combination you choose. For example my algorithm prefers 1*208=208 over 2*104=208 of the other algorithms which
doesn’t make a difference.

At 110 baud there is the only difference between my algorithm and Jonathan’s code for the commonly used baud rates
because I  pick 512*213 whereas he doesn’t use the 512 prescaler and therefore uses 1024*107 (both values are odd).

The reason why my algoorithm and Jonathan’s have different end results is because I use all possible prescaler combinations and he
doesn’t use the prescalers 64 and 512 which gives different results for baud rates around 100.

Please look at my comparison table at 
https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/patches/README.MD
which is created by the unit test
at https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/patches/check_baud_rates_unittest.c

baud    errOrig errMike errJon  pre*divOrig     pre*divMike     pre*divJon
46      -0.10%  -0.10%  -0.10%  1024*255        1024*255        1024*255
50      +0.16%  +0.16%  +0.16%  1024*234        1024*234        1024*234
75      +0.16%  +0.16%  +0.16%  1024*156        1024*156        1024*156
110     -0.44%  +0.03%  -0.44%  1024*107        512*213         1024*107
135     -0.22%  -0.22%  -0.22%  1024*87         512*174         1024*87
150     +0.16%  +0.16%  +0.16%  1024*78         512*156         1024*78
300     +0.16%  +0.16%  +0.16%  1024*39         512*78          1024*39
600     +0.16%  +0.16%  +0.16%  128*156         128*156         128*156
1200    +0.16%  +0.16%  +0.16%  128*78          64*156          128*78
1800    +0.16%  +0.16%  +0.16%  128*52          64*104          128*52
2400    +0.16%  +0.16%  +0.16%  128*39          64*78           128*39
4800    +0.16%  +0.16%  +0.16%  16*156          16*156          16*156
7200    +0.16%  +0.16%  +0.16%  16*104          8*208           16*104
9600    +0.16%  +0.16%  +0.16%  16*78           8*156           16*78
14400   +0.16%  +0.16%  +0.16%  16*52           8*104           16*52
19200   +0.16%  +0.16%  +0.16%  16*39           8*78            16*39
31250   +0.00%  +0.00%  +0.00%  2*192           2*192           2*192
38400   +0.16%  +0.16%  +0.16%  2*156           2*156           2*156
45450   +0.01%  +0.01%  +0.01%  2*132           2*132           2*132
56000   +0.13%  +0.13%  +0.13%  2*107           1*214           2*107
57600   +0.16%  +0.16%  +0.16%  2*104           1*208           2*104
76800   +0.16%  +0.16%  +0.16%  2*78            1*156           2*78
100000  +0.00%  +0.00%  +0.00%  2*60            1*120           2*60
115200  +0.16%  +0.16%  +0.16%  2*52            1*104           2*52
128000  -0.27%  -0.27%  -0.27%  2*47            1*94            2*47
153846  +0.00%  +0.00%  +0.00%  2*39            1*78            2*39
187500  +0.00%  +0.00%  +0.00%  2*32            1*64            2*32
230400  +0.16%  +0.16%  +0.16%  2*26            1*52            2*26
250000  +0.00%  +0.00%  +0.00%  2*24            1*48            2*24
256000  -2.34%  -0.27%  -0.27%  2*24            1*47            1*47
307200  -2.34%  +0.16%  +0.16%  2*20            1*39            1*39
460800  +0.16%  +0.16%  +0.16%  2*13            1*26            2*13
500000  +0.00%  +0.00%  +0.00%  2*12            1*24            2*12
750000  +0.00%  +0.00%  +0.00%  2*8             1*16            2*8
857143  -0.00%  -0.00%  -0.00%  2*7             1*14            2*7
921600  -6.99%  +0.16%  +0.16%  2*7             1*13            1*13
1000000 +0.00%  +0.00%  +0.00%  2*6             1*12            2*6
1090909 -8.33%  +0.00%  +0.00%  2*6             1*11            1*11
1200000 +0.00%  +0.00%  +0.00%  2*5             1*10            2*5
1333333 -10.00% +0.00%  +0.00%  2*5             1*9             1*9
1500000 +0.00%  +0.00%  +0.00%  2*4             2*4             2*4
2000000 +0.00%  +0.00%  +0.00%  2*3             2*3             2*3
3000000 +0.00%  +0.00%  +0.00%  2*2             2*2             2*2


baud means the requested baud rate
errOrig is the relative error of the resulting baud rate using the original code
errMike is the relative error of the resulting baud rate using my new code
errJon is the relative error of the resulting baud rate using Jonathan Olds' code
pre/divOrig the values of prescaler and divisor using the original code
pre/divMike the values of prescaler and divisor using my new code
pre/divJon the values of prescaler and divisor using Jonathan Olds' code

  Michael

Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Johan Hovold]

[ Please keep everyone on CC, and wrap your messages at 78 columns or
  so; I've reflown the text below. ]

On Fri, Nov 01, 2019 at 10:25:53AM +0100, Michael Dreher wrote:
> Hi Johan
> 
> >> https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/README.md
> >Interesting! From a very quick glance this looks very promising
> >(although he doesn't seem to handle rounding yet). I'll take a closer
> >look and compare with what I've done. 
> 
> That’s my code. I actually do rounding. It might not be obvious that
> the '(*2 + 1) / 2’ in  ((2UL * CH341_OSC_F) + / (prescaler *
> priv->baud_rate) + 1UL) / 2UL does a bankers rounding, that why I
> explained it in the README.MD:
> https://github.com/nospam2000/ch341-baudrate-calculation#rounding-issues

Yes, I noticed that today when taking closer look.

> > I also noticed that we should always pick the higher base clock if
> > the resulting divisor is odd (and within the valid range).
> 
> The general rule from the perspective of mathematics is to use the
> highest possible divider (and therefore the lowest possible prescaler)
> because that gives the lowest possible relative error in the division.
> As long as the result of divider*prescaler is the same it doesn’t
> matter which combination you choose. For example my algorithm prefers
> 1*208=208 over 2*104=208 of the other algorithms which doesn’t make a
> difference.

Indeed, I just mentioned this in reference to Jonathan's patch which
compared the two resulting rates with the third prescale register bit
set or cleared (effectively halving the rate). In this case, we can go
with either whenever the divisor is even. But generally, yes, a higher
divisor is better.

> At 110 baud there is the only difference between my algorithm and
> Jonathan’s code for the commonly used baud rates because I  pick
> 512*213 whereas he doesn’t use the 512 prescaler and therefore uses
> 1024*107 (both values are odd).

Yep, Jonathan only extended the current algorithm for the lowest
prescale value. I generalised it for all values, just like you did.

Note also that the third prescale bit affects divisor 2 through 8 for
all prescalers, not just when prescale bits 0 and 1 are zero
(prescaler=1 in your terminology) as your github doc claims.

It doesn't matter for your algorithm, just mentioning it for the sake
of completeness since I found this in my experiments.

Johan

RE: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Jonathan Olds]

Hi Johan,

> Thanks for testing. Do you mind if I add a tested-by tag for you to the
> patch?

Sure no problems. I tested it on a 64 bit I5 computer with the ch340g.
The Kernel was 5.0.0-32-generic.

> Not sure why that doesn't work, perhaps you just need to provide an
> appropriate "-p" option to strip the directory prefix?

I figured out the patching issue, my email client was removing some
line breaks.

> I suggest you use git directly instead. Clone Linus's repo at
> kernel.org, or use mine
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git
> 
> to get the latest USB serial changes.
> 
> Then you can apply patches directly from a mailbox using git-am (or from
> a patch using git-apply).

Thanks, will do. I didn't know about the git-am and git-apply commands.

Cheers,
Jonti

Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Johan Hovold]

On Fri, Nov 01, 2019 at 01:17:35PM +1300, Jonathan Olds wrote:
> Hi Johan,
> 
> Thanks for that.
> 
> I tested you patch out with the ch340g chip and it works fine for all
> rates I tried. I used a logic analyzer and made the following
> measurements...  
> 
> - wanted	measured error
> - 110		0.02%
> * 256000	0.26%
> - 576000	0.79%
> * 921600	0.16%
> * 1333333	0%
> - 2000000	0%
> - 3000000	0%
> 
> The asterisk are the ones that are really bad with the current Linux
> driver. Of the ones you that you mention my measurements match yours.
> So that looks really good.

Thanks for testing. Do you mind if I add a tested-by tag for you to the
patch?

> Yes the current Linux driver is a bit opaque.
> 
> BTW I had trouble patching the ch341.c file using your patch. Using...
> 
> ```
> wget
> https://raw.githubusercontent.com/torvalds/linux/master/drivers/usb/serial/c
> h341.c
> patch ch341.c patch.diff
> ```
> 
> where `patch.diff` was your patch I kept getting either
> "patch: **** unexpected end of file in patch" or
> "patch: **** malformed patch at line" depending what lines I changed.

Not sure why that doesn't work, perhaps you just need to provide an
appropriate "-p" option to strip the directory prefix?

I suggest you use git directly instead. Clone Linus's repo at
kernel.org, or use mine

	git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git

to get the latest USB serial changes.

Then you can apply patches directly from a mailbox using git-am (or from
a patch using git-apply).

> However it was easy enough to manually apply your patch by hand and
> that is what I did.

Ok, good. Thanks again for testing.

By the way, when communicating through the Linux mailing lists, try to
respond inline as I've done here instead of top-posting.

I'll try to look at the github repo you referred to and respin my patch
today.

Cheers,
Johan

RE: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Jonathan Olds]

Hi Johan,

Thanks for that.

I tested you patch out with the ch340g chip and it works fine for all
rates I tried. I used a logic analyzer and made the following
measurements...  

- wanted	measured error
- 110		0.02%
* 256000	0.26%
- 576000	0.79%
* 921600	0.16%
* 1333333	0%
- 2000000	0%
- 3000000	0%

The asterisk are the ones that are really bad with the current Linux
driver. Of the ones you that you mention my measurements match yours.
So that looks really good.

Yes the current Linux driver is a bit opaque.

BTW I had trouble patching the ch341.c file using your patch. Using...

```
wget
https://raw.githubusercontent.com/torvalds/linux/master/drivers/usb/serial/c
h341.c
patch ch341.c patch.diff
```

where `patch.diff` was your patch I kept getting either
"patch: **** unexpected end of file in patch" or
"patch: **** malformed patch at line" depending what lines I changed.

However it was easy enough to manually apply your patch by hand and
that is what I did.

Cheers,
Jonti
 

-----Original Message-----
From: Johan Hovold [mailto:johan@kernel.org] 
Sent: Wednesday, 30 October 2019 10:47 p.m.
To: Jonathan Olds
Cc: 'Johan Hovold'; linux-usb@vger.kernel.org;
frank@kingswood-consulting.co.uk; werner@cornelius-consult.de;
boris@hajduk.org
Subject: Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting
calculation

Hi Jonathan,

On Wed, Oct 30, 2019 at 08:18:48AM +1300, Jonathan Olds wrote:
> Hi Johan,

And apologies for the lack of feedback on this.

I've started looking into this a few times this fall, but got side
tracked. This past couple of weeks I did find some time, but never got
around to posting before heading to ELCE this week.

I've been working on a patch that generalises the vendor driver's
algorithm based on your findings and my own experiments. I've verified
it on both ch340g and ch341a, and it gives smaller (or equal) errors for
all rates, including the following standard rates

   - 1152000 (4.0% instead of 15% error)
   - 921600 (0.16% instead of -7.5%)
   - 576000 (-0.80% instead of -5.6%)
   - 200 (0.16% instead of -0.69%)
   - 134 (-0.05% instead of -0.63%)
   - 110 (0.03% instead of -0.44%)

but also on many non-standard ones by always picking the divisors with
smallest error.

My main issue with your proposed patch was that it was still based on the
current fairly opaque implementation, and then special-cased the higher
baud rates with some "magic" constants thrown in.

I also noticed that we should always pick the higher base clock if the
resulting divisor is odd (and within the valid range). No need to
determine the actual rates and compare; the odd divisor with the higher
base clock will always give a smaller error. If it's even, we can go
with the lower base clock and halve the divisor if we want.

When it comes to handling rounding, I opted for the brute force method
also used by the current vendor driver (the one you also referred to
earlier) by determining the actual rates for div and div + 1 and picking
the closest one. This should always give the smallest error, even
though we may not care about the midpoint rates for higher speeds that
much (the current algorithm would pick 2 Mbaud instead of 3 MBaud for
2950000, resulting in an error of -32% instead of 1.7%, etc).

> Michael has also produced a patch for the Linux kernel fixing the wrong
> baud rate calculations. This can be found here...
> 
>
https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/patches
> /Linux_4.14.114_ch341.patch
> 
> I haven't tested his code.
> 
> He has done a write up of his investigations about the registers here...
> 
>
https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/README.
> md

Interesting! From a very quick glance this looks very promising
(although he doesn't seem to handle rounding yet). I'll take a closer
look and compare with what I've done.

Would you mind giving the below patch a try meanwhile? It's still
lacking a proper commit message, and I wanted to take another look at it
when I'm back from the conference.

Just wanted to let you know that I'm on it and want to get this fixed in
one way or another for v5.4.

Thanks,
Johan


From 718c9f8d6f45014dc0c14a297d1017ecc945ef26 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Wed, 30 Oct 2019 09:56:09 +0100
Subject: [PATCH] USB: ch341: reimplement line-speed handling

Based on work by Jonathan Olds <jontio@i4free.co.nz> and the algorithm
used by the vendor driver.

FIXME: add commit message

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 97 +++++++++++++++++++++++++++++++-------
 1 file changed, 81 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 3bb1fff02bed..3dcc579b2d2f 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -144,37 +144,102 @@ static int ch341_control_in(struct usb_device *dev,
 	return 0;
 }
 
+#define CH341_CLKRATE		48000000
+#define CH341_CLK_DIV(ps, fact)	(1 << (12 - 3 * ps - fact))
+#define CH341_MIN_RATE(ps)	(CH341_CLKRATE / (CH341_CLK_DIV(ps, 1) *
512))
+
+static const speed_t ch341_min_rates[] = {
+	CH341_MIN_RATE(0),
+	CH341_MIN_RATE(1),
+	CH341_MIN_RATE(2),
+	CH341_MIN_RATE(3),
+};
+
+/*
+ * The device line speed is given by the following equation:
+ *
+ *	baud = 48000000 / (2^(12 - 3 * ps - fact) * div), where
+ *
+ *		0 <= ps <= 3,
+ *		0 <= fact <= 1,
+ *		2 <= div <= 256 if fact = 0, or
+ *		9 <= div <= 256 if fact = 1
+ */
+static int ch341_get_divisor(speed_t speed)
+{
+	unsigned int fact, div, clk_div;
+	int ps;
+
+	/*
+	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
+	 * sanity checks below redundant.
+	 */
+	speed = clamp(speed, 46U, 3000000U);
+
+	/*
+	 * Start with highest possible base clock (fact = 1) that will give
a
+	 * divisor strictly less than 512.
+	 */
+	fact = 1;
+	for (ps = 3; ps >= 0; ps--) {
+		if (speed > ch341_min_rates[ps])
+			break;
+	}
+
+	if (ps < 0)
+		return -EINVAL;
+
+	/* Determine corresponding divisor, rounding down. */
+	clk_div = CH341_CLK_DIV(ps, fact);
+	div = CH341_CLKRATE / (clk_div * speed);
+
+	/* Halve base clock (fact = 0) if required. */
+	if (div < 9 || div > 255) {
+		div >>= 1;
+		clk_div <<= 1;
+		fact = 0;
+	}
+
+	if (div < 2)
+		return -EINVAL;
+
+	/*
+	 * Pick next divisor if resulting rate is closer to the requested
one,
+	 * scale up to avoid rounding errors on low rates.
+	 */
+	if (16 * CH341_CLKRATE / (clk_div * div) - 16 * speed >=
+			16 * speed - 16 * CH341_CLKRATE / (clk_div * (div +
1)))
+		div++;
+
+	/* Prefer lower base clock (fact = 0) if even divisor. */
+	if (fact == 1 && div % 2 == 0) {
+		div >>= 1;
+		fact = 0;
+	}
+
+	return (0x100 - div) << 8 | fact << 2 | ps;
+}
+
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
 				  struct ch341_private *priv, u8 lcr)
 {
-	short a;
+	int val;
 	int r;
-	unsigned long factor;
-	short divisor;
 
 	if (!priv->baud_rate)
 		return -EINVAL;
-	factor = (CH341_BAUDBASE_FACTOR / priv->baud_rate);
-	divisor = CH341_BAUDBASE_DIVMAX;
 
-	while ((factor > 0xfff0) && divisor) {
-		factor >>= 3;
-		divisor--;
-	}
-
-	if (factor > 0xfff0)
+	val = ch341_get_divisor(priv->baud_rate);
+	if (val < 0)
 		return -EINVAL;
 
-	factor = 0x10000 - factor;
-	a = (factor & 0xff00) | divisor;
-
 	/*
 	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
 	 * has been received unless bit 7 is set.
 	 */
-	a |= BIT(7);
+	val |= BIT(7);
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, a);
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
 	if (r)
 		return r;
 
-- 
2.23.0

Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Johan Hovold]

Hi Jonathan,

On Wed, Oct 30, 2019 at 08:18:48AM +1300, Jonathan Olds wrote:
> Hi Johan,

And apologies for the lack of feedback on this.

I've started looking into this a few times this fall, but got side
tracked. This past couple of weeks I did find some time, but never got
around to posting before heading to ELCE this week.

I've been working on a patch that generalises the vendor driver's
algorithm based on your findings and my own experiments. I've verified
it on both ch340g and ch341a, and it gives smaller (or equal) errors for
all rates, including the following standard rates

   - 1152000 (4.0% instead of 15% error)
   - 921600 (0.16% instead of -7.5%)
   - 576000 (-0.80% instead of -5.6%)
   - 200 (0.16% instead of -0.69%)
   - 134 (-0.05% instead of -0.63%)
   - 110 (0.03% instead of -0.44%)

but also on many non-standard ones by always picking the divisors with
smallest error.

My main issue with your proposed patch was that it was still based on the
current fairly opaque implementation, and then special-cased the higher
baud rates with some "magic" constants thrown in.

I also noticed that we should always pick the higher base clock if the
resulting divisor is odd (and within the valid range). No need to
determine the actual rates and compare; the odd divisor with the higher
base clock will always give a smaller error. If it's even, we can go
with the lower base clock and halve the divisor if we want.

When it comes to handling rounding, I opted for the brute force method
also used by the current vendor driver (the one you also referred to
earlier) by determining the actual rates for div and div + 1 and picking
the closest one. This should always give the smallest error, even
though we may not care about the midpoint rates for higher speeds that
much (the current algorithm would pick 2 Mbaud instead of 3 MBaud for
2950000, resulting in an error of -32% instead of 1.7%, etc).

> Michael has also produced a patch for the Linux kernel fixing the wrong
> baud rate calculations. This can be found here...
> 
> https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/patches
> /Linux_4.14.114_ch341.patch
> 
> I haven't tested his code.
> 
> He has done a write up of his investigations about the registers here...
> 
> https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/README.
> md

Interesting! From a very quick glance this looks very promising
(although he doesn't seem to handle rounding yet). I'll take a closer
look and compare with what I've done.

Would you mind giving the below patch a try meanwhile? It's still
lacking a proper commit message, and I wanted to take another look at it
when I'm back from the conference.

Just wanted to let you know that I'm on it and want to get this fixed in
one way or another for v5.4.

Thanks,
Johan


From 718c9f8d6f45014dc0c14a297d1017ecc945ef26 Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Wed, 30 Oct 2019 09:56:09 +0100
Subject: [PATCH] USB: ch341: reimplement line-speed handling

Based on work by Jonathan Olds <jontio@i4free.co.nz> and the algorithm
used by the vendor driver.

FIXME: add commit message

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ch341.c | 97 +++++++++++++++++++++++++++++++-------
 1 file changed, 81 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 3bb1fff02bed..3dcc579b2d2f 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -144,37 +144,102 @@ static int ch341_control_in(struct usb_device *dev,
 	return 0;
 }
 
+#define CH341_CLKRATE		48000000
+#define CH341_CLK_DIV(ps, fact)	(1 << (12 - 3 * ps - fact))
+#define CH341_MIN_RATE(ps)	(CH341_CLKRATE / (CH341_CLK_DIV(ps, 1) * 512))
+
+static const speed_t ch341_min_rates[] = {
+	CH341_MIN_RATE(0),
+	CH341_MIN_RATE(1),
+	CH341_MIN_RATE(2),
+	CH341_MIN_RATE(3),
+};
+
+/*
+ * The device line speed is given by the following equation:
+ *
+ *	baud = 48000000 / (2^(12 - 3 * ps - fact) * div), where
+ *
+ *		0 <= ps <= 3,
+ *		0 <= fact <= 1,
+ *		2 <= div <= 256 if fact = 0, or
+ *		9 <= div <= 256 if fact = 1
+ */
+static int ch341_get_divisor(speed_t speed)
+{
+	unsigned int fact, div, clk_div;
+	int ps;
+
+	/*
+	 * Clamp to supported range, this makes the (ps < 0) and (div < 2)
+	 * sanity checks below redundant.
+	 */
+	speed = clamp(speed, 46U, 3000000U);
+
+	/*
+	 * Start with highest possible base clock (fact = 1) that will give a
+	 * divisor strictly less than 512.
+	 */
+	fact = 1;
+	for (ps = 3; ps >= 0; ps--) {
+		if (speed > ch341_min_rates[ps])
+			break;
+	}
+
+	if (ps < 0)
+		return -EINVAL;
+
+	/* Determine corresponding divisor, rounding down. */
+	clk_div = CH341_CLK_DIV(ps, fact);
+	div = CH341_CLKRATE / (clk_div * speed);
+
+	/* Halve base clock (fact = 0) if required. */
+	if (div < 9 || div > 255) {
+		div >>= 1;
+		clk_div <<= 1;
+		fact = 0;
+	}
+
+	if (div < 2)
+		return -EINVAL;
+
+	/*
+	 * Pick next divisor if resulting rate is closer to the requested one,
+	 * scale up to avoid rounding errors on low rates.
+	 */
+	if (16 * CH341_CLKRATE / (clk_div * div) - 16 * speed >=
+			16 * speed - 16 * CH341_CLKRATE / (clk_div * (div + 1)))
+		div++;
+
+	/* Prefer lower base clock (fact = 0) if even divisor. */
+	if (fact == 1 && div % 2 == 0) {
+		div >>= 1;
+		fact = 0;
+	}
+
+	return (0x100 - div) << 8 | fact << 2 | ps;
+}
+
 static int ch341_set_baudrate_lcr(struct usb_device *dev,
 				  struct ch341_private *priv, u8 lcr)
 {
-	short a;
+	int val;
 	int r;
-	unsigned long factor;
-	short divisor;
 
 	if (!priv->baud_rate)
 		return -EINVAL;
-	factor = (CH341_BAUDBASE_FACTOR / priv->baud_rate);
-	divisor = CH341_BAUDBASE_DIVMAX;
 
-	while ((factor > 0xfff0) && divisor) {
-		factor >>= 3;
-		divisor--;
-	}
-
-	if (factor > 0xfff0)
+	val = ch341_get_divisor(priv->baud_rate);
+	if (val < 0)
 		return -EINVAL;
 
-	factor = 0x10000 - factor;
-	a = (factor & 0xff00) | divisor;
-
 	/*
 	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
 	 * has been received unless bit 7 is set.
 	 */
-	a |= BIT(7);
+	val |= BIT(7);
 
-	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, a);
+	r = ch341_control_out(dev, CH341_REQ_WRITE_REG, 0x1312, val);
 	if (r)
 		return r;
 
-- 
2.23.0

RE: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Jonathan Olds]

Hi Johan,

Michael has also produced a patch for the Linux kernel fixing the wrong
baud rate calculations. This can be found here...

https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/patches
/Linux_4.14.114_ch341.patch

I haven't tested his code.

He has done a write up of his investigations about the registers here...

https://github.com/nospam2000/ch341-baudrate-calculation/blob/master/README.
md

Cheers,
Jonti 

-----Original Message-----
From: Jonti Olds [mailto:joldsphone@gmail.com] On Behalf Of Jonathan Olds
Sent: Saturday, 29 June 2019 12:05 p.m.
To: 'Johan Hovold'
Cc: linux-usb@vger.kernel.org; frank@kingswood-consulting.co.uk;
werner@cornelius-consult.de; boris@hajduk.org; 'Jonathan Olds'
Subject: RE: [PATCH] USB: serial: ch341: fix wrong baud rate setting
calculation

Hi Johan,

Sorry for the slow reply. Thanks for the feedback.

I've amended the patch and is below. I think I've done all of your
suggestions and looks more Linux Kernelish style.

> drop the denom outmost parenthesis (also in some expressions below)

I removed the outmost parenthesis from the " denom = " line but wasn't too
sure what the "some expressions below" were. 

int only guarantees 16bits so that's why I went for ones of the form
long/s32/int32_t for some variables. I noticed that other things like
"priv->baud_rate" for example is an unsigned int so that wouldn't work for
some 16bit systems (such as the dsPIC with xc16) for speeds beyond 65KBaud.

Here's the amended patch, any feedback greatly appreciated...

For some wanted baud rates ch341_set_baudrate_lcr() calculates the "a"
value such that it produces a significantly different baud rate than the
desired one. This means some hardware can't communicate with the CH34x
chip. Particularly obvious wrong baud rates are 256000 and 921600 which
deviate by 2.3% and 7.4% respectively. This proposed patch will bring the
errors for these baud rates to below 0.5%. This patch will significantly
improve the error of some other unusual baud rates too (such as 1333333
from 10% error to 0% error). Currently ch341_set_baudrate_lcr() will
accept any baud rate and can produce a practically arbitrary large error
(for example a 40% error for 5000000). This patch does not address this
issue.

The patch has been tested on two computers and only with the CH340G chip.

With a CH340G chip with "Chip version: 0x31" on a JPIC I tested the
bitrate error using a RIGOL DS1054 scope without the patch and measured
the following...

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%

Measurements and working as a libre/open office document can be found at
https://jontio.github.io/linux_kernel_work/ch43x_tests.ods

Signed-off-by: Jonathan Olds <jontio@i4free.co.nz>
---
 drivers/usb/serial/ch341.c | 46 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 3bb1fff02bed..5ef8db2974d5 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -54,6 +54,11 @@
 #define CH341_BAUDBASE_FACTOR 1532620800
 #define CH341_BAUDBASE_DIVMAX 3
 
+/* Chip frequency is 12Mhz. not quite the same as
+ * (CH341_BAUDBASE_FACTOR>>7)
+ */
+#define CH341_OSC_FREQUENCY 12000000
+
 /* Break support - the information used to implement this was gleaned from
  * the Net/FreeBSD uchcom.c driver by Takanori Watanabe.  Domo arigato.
  */
@@ -151,6 +156,9 @@ static int ch341_set_baudrate_lcr(struct usb_device
*dev,
 	int r;
 	unsigned long factor;
 	short divisor;
+	u8 msb;
+	s32 baud_wanted;
+	u32 denom;
 
 	if (!priv->baud_rate)
 		return -EINVAL;
@@ -168,6 +176,44 @@ static int ch341_set_baudrate_lcr(struct usb_device
*dev,
 	factor = 0x10000 - factor;
 	a = (factor & 0xff00) | divisor;
 
+	/*
+	 * Calculate baud error using the 0,1,2,3 lsb and
+	 * also the error without the divisor (lsb==7).
+	 * Decide whether the divisor should be used.
+	 */
+	msb = (a >> 8) & 0xff;
+	baud_wanted = priv->baud_rate;
+	denom = (1ul << (10 - 3 * (divisor & 0x03))) * (256 - msb);
+	/*
+	 * baud_wanted==(CH341_OSC_FREQUENCY/256) implies msb==0 for no
divisor
+	 * the 100 is for rounding.
+	 */
+	if (denom && ((baud_wanted + 100) >= (((u32)CH341_OSC_FREQUENCY) >>
8))) {
+		/* Calculate error for divisor */
+		long baud_expected = ((u32)CH341_OSC_FREQUENCY) / denom;
+		u32 baud_error_difference = abs(baud_expected-baud_wanted);
+
+		/* Calculate a for no divisor */
+		u32 a_no_divisor = ((0x10000 - (((u32)CH341_OSC_FREQUENCY)
<< 8) /
+			baud_wanted + 128) & 0xff00) | 0x07;
+
+		/* a_no_divisor is only valid for msb<248 */
+		if ((a_no_divisor >> 8) < 248) {
+			/* Calculate error for no divisor */
+			long baud_expected_no_divisor =
((u32)CH341_OSC_FREQUENCY) /
+				(256 - (a_no_divisor >> 8));
+			u32 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;
+		}
+	}
+
 	/*
 	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
 	 * has been received unless bit 7 is set.
-- 
2.17.1

Cheers,
Jonti

-----Original Message-----
From: Johan Hovold [mailto:johan@kernel.org] 
Sent: Friday, 21 June 2019 1:43 a.m.
To: jontio
Cc: johan@kernel.org; linux-usb@vger.kernel.org
Subject: Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting
calculation

On Sat, Jun 08, 2019 at 05:13:09PM +1200, jontio wrote:
> For some wanted baud rates ch341_set_baudrate_lcr() calculates the "a"
> value such that it produces a significantly different baud rate than 
> the desired one. This means some hardware can't communicate with the 
> CH34x chip. Particularly obvious wrong baud rates are 256000 and 
> 921600 which deviate by 2.3% and 7.4% respectively. This proposed 
> patch will bring the errors for these baud rates to below 0.5%. This 
> patch will significantly improve the error of some other unusual baud 
> rates too (such as 1333333 from 10% error to 0% error). Currently 
> ch341_set_baudrate_lcr() will accept any baud rate and can produce a 
> practically arbitrary large error (for example a 40% error for 
> 5000000) this patch does not address this issue.

It doesn't hurt to expand the commit message with further details from your
other mail.

> Signed-off-by: jontio <jontio@i4free.co.nz>

You need to sign off with your full name. It should also match the From line
(author).

> ---
>  drivers/usb/serial/ch341.c | 45 
> ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c 
> index 3bb1fff02bed..7cd1d6f70b56 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -54,6 +54,9 @@
>  #define CH341_BAUDBASE_FACTOR 1532620800  #define 
> CH341_BAUDBASE_DIVMAX 3
>  
> +/* Chip frequency is 12Mhz. not quite the same as 
> +(CH341_BAUDBASE_FACTOR>>7) */ #define CH341_OSC_FREQUENCY 12000000
> +
>  /* Break support - the information used to implement this was gleaned
from
>   * the Net/FreeBSD uchcom.c driver by Takanori Watanabe.  Domo arigato.
>   */
> @@ -168,6 +171,48 @@ static int ch341_set_baudrate_lcr(struct usb_device
*dev,
>  	factor = 0x10000 - factor;
>  	a = (factor & 0xff00) | divisor;
>  
> +	/*
> +	 * Calculate baud error using the 0,1,2,3 LSB and
> +	 * also the error without the divisor (LSB==7).
> +	 * Decide whether the divisor should be used.

Wrap also comments at 72 cols or so.

> +	 */
> +	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));

It's not obvious from just looking at the above chunk that 3*lsB < 10.

And some style issues:

 - declare variables at the start of the function (or possibly start of
   block), and defer non-trivial initialisation
 - use the kernel types u32, s32 or plain (unsigned) int instead of the
   c99 types.
 - no camel case, msb, lsb is fine
 - add a space on both sides of operators (also in your comments)
 - drop the denom outmost parenthesis (also in some expressions below)
 - please use lowercase hex notation for consistency with the rest of
   the driver (function)

> +	/*
> +	 * baud_wanted==(CH341_OSC_FREQUENCY/256) implies MSB==0 for no
divisor
> +	 * the 100 is for rounding.
> +	 */
> +	if (denom && ((baud_wanted+100) >= 
> +(((uint32_t)CH341_OSC_FREQUENCY)>>8))) {
> +
> +		/* Calculate error for divisor */
> +		int32_t baud_expected = ((uint32_t)CH341_OSC_FREQUENCY) /
denom;
> +		uint32_t baud_error_difference =
abs(baud_expected-baud_wanted);
> +
> +		/* Calculate a for no divisor */
> +		uint32_t a_no_divisor =
((0x10000-(((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) {
> +
> +			/* Calculate 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;
> +		}
> +

Stray newline.

> +	}
> +
>  	/*
>  	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
>  	 * has been received unless bit 7 is set.

Ok, I'm gonna have to look at this again, but perhaps you can consider the
style input meanwhile.

Johan

RE: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Jonathan Olds]

Hi Johan,

Sorry for the slow reply. Thanks for the feedback.

I've amended the patch and is below. I think I've done all of your
suggestions and looks more Linux Kernelish style.

> drop the denom outmost parenthesis (also in some expressions below)

I removed the outmost parenthesis from the " denom = " line but wasn't too
sure what the "some expressions below" were. 

int only guarantees 16bits so that's why I went for ones of the form
long/s32/int32_t for some variables. I noticed that other things like
"priv->baud_rate" for example is an unsigned int so that wouldn't work for
some 16bit systems (such as the dsPIC with xc16) for speeds beyond 65KBaud.

Here's the amended patch, any feedback greatly appreciated...

For some wanted baud rates ch341_set_baudrate_lcr() calculates the "a"
value such that it produces a significantly different baud rate than the
desired one. This means some hardware can't communicate with the CH34x
chip. Particularly obvious wrong baud rates are 256000 and 921600 which
deviate by 2.3% and 7.4% respectively. This proposed patch will bring the
errors for these baud rates to below 0.5%. This patch will significantly
improve the error of some other unusual baud rates too (such as 1333333
from 10% error to 0% error). Currently ch341_set_baudrate_lcr() will
accept any baud rate and can produce a practically arbitrary large error
(for example a 40% error for 5000000). This patch does not address this
issue.

The patch has been tested on two computers and only with the CH340G chip.

With a CH340G chip with "Chip version: 0x31" on a JPIC I tested the
bitrate error using a RIGOL DS1054 scope without the patch and measured
the following...

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%

Measurements and working as a libre/open office document can be found at
https://jontio.github.io/linux_kernel_work/ch43x_tests.ods

Signed-off-by: Jonathan Olds <jontio@i4free.co.nz>
---
 drivers/usb/serial/ch341.c | 46 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 3bb1fff02bed..5ef8db2974d5 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -54,6 +54,11 @@
 #define CH341_BAUDBASE_FACTOR 1532620800
 #define CH341_BAUDBASE_DIVMAX 3
 
+/* Chip frequency is 12Mhz. not quite the same as
+ * (CH341_BAUDBASE_FACTOR>>7)
+ */
+#define CH341_OSC_FREQUENCY 12000000
+
 /* Break support - the information used to implement this was gleaned from
  * the Net/FreeBSD uchcom.c driver by Takanori Watanabe.  Domo arigato.
  */
@@ -151,6 +156,9 @@ static int ch341_set_baudrate_lcr(struct usb_device
*dev,
 	int r;
 	unsigned long factor;
 	short divisor;
+	u8 msb;
+	s32 baud_wanted;
+	u32 denom;
 
 	if (!priv->baud_rate)
 		return -EINVAL;
@@ -168,6 +176,44 @@ static int ch341_set_baudrate_lcr(struct usb_device
*dev,
 	factor = 0x10000 - factor;
 	a = (factor & 0xff00) | divisor;
 
+	/*
+	 * Calculate baud error using the 0,1,2,3 lsb and
+	 * also the error without the divisor (lsb==7).
+	 * Decide whether the divisor should be used.
+	 */
+	msb = (a >> 8) & 0xff;
+	baud_wanted = priv->baud_rate;
+	denom = (1ul << (10 - 3 * (divisor & 0x03))) * (256 - msb);
+	/*
+	 * baud_wanted==(CH341_OSC_FREQUENCY/256) implies msb==0 for no
divisor
+	 * the 100 is for rounding.
+	 */
+	if (denom && ((baud_wanted + 100) >= (((u32)CH341_OSC_FREQUENCY) >>
8))) {
+		/* Calculate error for divisor */
+		long baud_expected = ((u32)CH341_OSC_FREQUENCY) / denom;
+		u32 baud_error_difference = abs(baud_expected-baud_wanted);
+
+		/* Calculate a for no divisor */
+		u32 a_no_divisor = ((0x10000 - (((u32)CH341_OSC_FREQUENCY)
<< 8) /
+			baud_wanted + 128) & 0xff00) | 0x07;
+
+		/* a_no_divisor is only valid for msb<248 */
+		if ((a_no_divisor >> 8) < 248) {
+			/* Calculate error for no divisor */
+			long baud_expected_no_divisor =
((u32)CH341_OSC_FREQUENCY) /
+				(256 - (a_no_divisor >> 8));
+			u32 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;
+		}
+	}
+
 	/*
 	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
 	 * has been received unless bit 7 is set.
-- 
2.17.1

Cheers,
Jonti

-----Original Message-----
From: Johan Hovold [mailto:johan@kernel.org] 
Sent: Friday, 21 June 2019 1:43 a.m.
To: jontio
Cc: johan@kernel.org; linux-usb@vger.kernel.org
Subject: Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting
calculation

On Sat, Jun 08, 2019 at 05:13:09PM +1200, jontio wrote:
> For some wanted baud rates ch341_set_baudrate_lcr() calculates the "a"
> value such that it produces a significantly different baud rate than 
> the desired one. This means some hardware can't communicate with the 
> CH34x chip. Particularly obvious wrong baud rates are 256000 and 
> 921600 which deviate by 2.3% and 7.4% respectively. This proposed 
> patch will bring the errors for these baud rates to below 0.5%. This 
> patch will significantly improve the error of some other unusual baud 
> rates too (such as 1333333 from 10% error to 0% error). Currently 
> ch341_set_baudrate_lcr() will accept any baud rate and can produce a 
> practically arbitrary large error (for example a 40% error for 
> 5000000) this patch does not address this issue.

It doesn't hurt to expand the commit message with further details from your
other mail.

> Signed-off-by: jontio <jontio@i4free.co.nz>

You need to sign off with your full name. It should also match the From line
(author).

> ---
>  drivers/usb/serial/ch341.c | 45 
> ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c 
> index 3bb1fff02bed..7cd1d6f70b56 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -54,6 +54,9 @@
>  #define CH341_BAUDBASE_FACTOR 1532620800  #define 
> CH341_BAUDBASE_DIVMAX 3
>  
> +/* Chip frequency is 12Mhz. not quite the same as 
> +(CH341_BAUDBASE_FACTOR>>7) */ #define CH341_OSC_FREQUENCY 12000000
> +
>  /* Break support - the information used to implement this was gleaned
from
>   * the Net/FreeBSD uchcom.c driver by Takanori Watanabe.  Domo arigato.
>   */
> @@ -168,6 +171,48 @@ static int ch341_set_baudrate_lcr(struct usb_device
*dev,
>  	factor = 0x10000 - factor;
>  	a = (factor & 0xff00) | divisor;
>  
> +	/*
> +	 * Calculate baud error using the 0,1,2,3 LSB and
> +	 * also the error without the divisor (LSB==7).
> +	 * Decide whether the divisor should be used.

Wrap also comments at 72 cols or so.

> +	 */
> +	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));

It's not obvious from just looking at the above chunk that 3*lsB < 10.

And some style issues:

 - declare variables at the start of the function (or possibly start of
   block), and defer non-trivial initialisation
 - use the kernel types u32, s32 or plain (unsigned) int instead of the
   c99 types.
 - no camel case, msb, lsb is fine
 - add a space on both sides of operators (also in your comments)
 - drop the denom outmost parenthesis (also in some expressions below)
 - please use lowercase hex notation for consistency with the rest of
   the driver (function)

> +	/*
> +	 * baud_wanted==(CH341_OSC_FREQUENCY/256) implies MSB==0 for no
divisor
> +	 * the 100 is for rounding.
> +	 */
> +	if (denom && ((baud_wanted+100) >= 
> +(((uint32_t)CH341_OSC_FREQUENCY)>>8))) {
> +
> +		/* Calculate error for divisor */
> +		int32_t baud_expected = ((uint32_t)CH341_OSC_FREQUENCY) /
denom;
> +		uint32_t baud_error_difference =
abs(baud_expected-baud_wanted);
> +
> +		/* Calculate a for no divisor */
> +		uint32_t a_no_divisor =
((0x10000-(((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) {
> +
> +			/* Calculate 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;
> +		}
> +

Stray newline.

> +	}
> +
>  	/*
>  	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
>  	 * has been received unless bit 7 is set.

Ok, I'm gonna have to look at this again, but perhaps you can consider the
style input meanwhile.

Johan

Re: [PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[Johan Hovold]

On Sat, Jun 08, 2019 at 05:13:09PM +1200, jontio wrote:
> For some wanted baud rates ch341_set_baudrate_lcr() calculates the "a"
> value such that it produces a significantly different baud rate than the
> desired one. This means some hardware can't communicate with the CH34x
> chip. Particularly obvious wrong baud rates are 256000 and 921600 which
> deviate by 2.3% and 7.4% respectively. This proposed patch will bring the
> errors for these baud rates to below 0.5%. This patch will significantly
> improve the error of some other unusual baud rates too (such as 1333333
> from 10% error to 0% error). Currently ch341_set_baudrate_lcr() will
> accept any baud rate and can produce a practically arbitrary large error
> (for example a 40% error for 5000000) this patch does not address this
> issue.

It doesn't hurt to expand the commit message with further details from
your other mail.

> Signed-off-by: jontio <jontio@i4free.co.nz>

You need to sign off with your full name. It should also match the From
line (author).

> ---
>  drivers/usb/serial/ch341.c | 45 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index 3bb1fff02bed..7cd1d6f70b56 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -54,6 +54,9 @@
>  #define CH341_BAUDBASE_FACTOR 1532620800
>  #define CH341_BAUDBASE_DIVMAX 3
>  
> +/* Chip frequency is 12Mhz. not quite the same as (CH341_BAUDBASE_FACTOR>>7) */
> +#define CH341_OSC_FREQUENCY 12000000
> +
>  /* Break support - the information used to implement this was gleaned from
>   * the Net/FreeBSD uchcom.c driver by Takanori Watanabe.  Domo arigato.
>   */
> @@ -168,6 +171,48 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
>  	factor = 0x10000 - factor;
>  	a = (factor & 0xff00) | divisor;
>  
> +	/*
> +	 * Calculate baud error using the 0,1,2,3 LSB and
> +	 * also the error without the divisor (LSB==7).
> +	 * Decide whether the divisor should be used.

Wrap also comments at 72 cols or so.

> +	 */
> +	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));

It's not obvious from just looking at the above chunk that 3*lsB < 10.

And some style issues:

 - declare variables at the start of the function (or possibly start of
   block), and defer non-trivial initialisation
 - use the kernel types u32, s32 or plain (unsigned) int instead of the
   c99 types.
 - no camel case, msb, lsb is fine
 - add a space on both sides of operators (also in your comments)
 - drop the denom outmost parenthesis (also in some expressions below)
 - please use lowercase hex notation for consistency with the rest of
   the driver (function)

> +	/*
> +	 * baud_wanted==(CH341_OSC_FREQUENCY/256) implies MSB==0 for no divisor
> +	 * the 100 is for rounding.
> +	 */
> +	if (denom && ((baud_wanted+100) >= (((uint32_t)CH341_OSC_FREQUENCY)>>8))) {
> +
> +		/* Calculate error for divisor */
> +		int32_t baud_expected = ((uint32_t)CH341_OSC_FREQUENCY) / denom;
> +		uint32_t baud_error_difference = abs(baud_expected-baud_wanted);
> +
> +		/* Calculate a for no divisor */
> +		uint32_t a_no_divisor = ((0x10000-(((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) {
> +
> +			/* Calculate 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;
> +		}
> +

Stray newline.

> +	}
> +
>  	/*
>  	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
>  	 * has been received unless bit 7 is set.

Ok, I'm gonna have to look at this again, but perhaps you can consider
the style input meanwhile.

Johan

[PATCH] USB: serial: ch341: fix wrong baud rate setting calculation

[jontio]

For some wanted baud rates ch341_set_baudrate_lcr() calculates the "a"
value such that it produces a significantly different baud rate than the
desired one. This means some hardware can't communicate with the CH34x
chip. Particularly obvious wrong baud rates are 256000 and 921600 which
deviate by 2.3% and 7.4% respectively. This proposed patch will bring the
errors for these baud rates to below 0.5%. This patch will significantly
improve the error of some other unusual baud rates too (such as 1333333
from 10% error to 0% error). Currently ch341_set_baudrate_lcr() will
accept any baud rate and can produce a practically arbitrary large error
(for example a 40% error for 5000000) this patch does not address this
issue.

Signed-off-by: jontio <jontio@i4free.co.nz>
---
 drivers/usb/serial/ch341.c | 45 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
index 3bb1fff02bed..7cd1d6f70b56 100644
--- a/drivers/usb/serial/ch341.c
+++ b/drivers/usb/serial/ch341.c
@@ -54,6 +54,9 @@
 #define CH341_BAUDBASE_FACTOR 1532620800
 #define CH341_BAUDBASE_DIVMAX 3
 
+/* Chip frequency is 12Mhz. not quite the same as (CH341_BAUDBASE_FACTOR>>7) */
+#define CH341_OSC_FREQUENCY 12000000
+
 /* Break support - the information used to implement this was gleaned from
  * the Net/FreeBSD uchcom.c driver by Takanori Watanabe.  Domo arigato.
  */
@@ -168,6 +171,48 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev,
 	factor = 0x10000 - factor;
 	a = (factor & 0xff00) | divisor;
 
+	/*
+	 * Calculate baud error using the 0,1,2,3 LSB and
+	 * also the error without the divisor (LSB==7).
+	 * Decide whether the divisor should be used.
+	 */
+	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));
+	/*
+	 * baud_wanted==(CH341_OSC_FREQUENCY/256) implies MSB==0 for no divisor
+	 * the 100 is for rounding.
+	 */
+	if (denom && ((baud_wanted+100) >= (((uint32_t)CH341_OSC_FREQUENCY)>>8))) {
+
+		/* Calculate error for divisor */
+		int32_t baud_expected = ((uint32_t)CH341_OSC_FREQUENCY) / denom;
+		uint32_t baud_error_difference = abs(baud_expected-baud_wanted);
+
+		/* Calculate a for no divisor */
+		uint32_t a_no_divisor = ((0x10000-(((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) {
+
+			/* Calculate 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;
+		}
+
+	}
+
 	/*
 	 * CH341A buffers data until a full endpoint-size packet (32 bytes)
 	 * has been received unless bit 7 is set.
-- 
2.17.1