Re: [PATCH v2] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
On Wed, Jul 15, 2015 at 01:35:58PM +0200, Michał Pecio wrote:
This commit fixes the following issues:
1. The 9th bit of buf was believed to be the LSB of divisor's
exponent, but the hardware interprets it as MSB (9th bit) of the
mantissa. The exponent is actually one bit shorter and applies
to base 4, not 2 as previously believed.
2. Loop iterations doubled the exponent instead of incrementing.
3. The exponent wasn't checked for overflow.
4. The function returned requested rate instead of actual rate.
Due to issue #2, the old code deviated from the wrong formula
described in #1 and actually yielded correct rates when divisor
was lower than 4096 by using exponents of 0, 2 or 4 base-2,
interpreted as 0, 1, 2 base-4 with the 9th mantissa bit clear.
However, at 93.75 kbaud or less the rate turned out too slow
due to #2 or too fast due to #2 and #3.
I tested this patch by sending and validating 0x00,0x01,..,0xff
to an FTDI dongle at 234, 987, 2401, 9601, 31415, 115199, 250k,
500k, 750k, 1M, 1.5M, 3M+1 baud. All rates passed.
I also used pv to check speed at some rates unsupported by FTDI:
45 (the lowest possible), 2M, 4M, 5M and 6M-1. Looked sane.
Signed-off-by: Michal Pecio michal.pe...@gmail.com
I took another look at this one and discovered an issue that should be
addressed.
Pointing out two style issues I had fixed locally as well.
---
drivers/usb/serial/pl2303.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index f5257af..3f9a808 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -362,22 +362,36 @@ static speed_t pl2303_encode_baud_rate_direct(unsigned
char buf[4],
static speed_t pl2303_encode_baud_rate_divisor(unsigned char buf[4],
speed_t baud)
{
- unsigned int tmp;
+ u32 baseline, mantissa, exponent;
Please keep these as unsigned int.
/*
* Apparently the formula is:
- * baudrate = 12M * 32 / (2^buf[1]) / buf[0]
+ * baudrate = 12M * 32 / (mantissa * 4^exponent)
+ * where
+ * mantissa = buf[8:0]
+ * exponent = buf[11:9]
*/
- tmp = 1200 * 32 / baud;
+ baseline = 1200 * 32;
+ mantissa = baseline / baud;
+ exponent = 0;
+ while (mantissa = 512) {
+ if (exponent 7) {
+ mantissa = 2; /* divide by 4 */
+ exponent++;
+ } else {
+ /* Exponent is maxed. Trim mantissa and leave. */
+ mantissa = 511;
+ break;
+ }
+ }
+
buf[3] = 0x80;
buf[2] = 0;
- buf[1] = (tmp = 256);
- while (tmp = 256) {
- tmp = 2;
- buf[1] = 1;
- }
- buf[0] = tmp;
+ buf[1] = exponent 1 | mantissa 8;
+ buf[0] = mantissa 0xff;
+ /* Calculate and return the exact baud rate. */
+ baud = (baseline / mantissa) (exponent 1);
You should handle division by zero here. It cannot currently happen as
we cap the baudrate, but you should handle it here nonetheless. You can
still assume a non-zero baudrate, though.
Please add a newline before returning as well.
return baud;
}
Thanks,
Johan
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
+ u32 baseline, mantissa, exponent; Please keep these as unsigned int. What's the reason for that? u32 is the exact width needed to perform these computations, while unsigned int is something a bit unspecified. + /* Calculate and return the exact baud rate. */ + baud = (baseline / mantissa) (exponent 1); You should handle division by zero here. It cannot currently happen as we cap the baudrate, but you should handle it here nonetheless. You can still assume a non-zero baudrate, though. How about that? mantissa = baseline / baud; if (mantissa == 0) mantissa = 1; Writing zero to the chip as it's currently done probably doesn't make much sense anyway... -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
On Thu, Jul 23, 2015 at 04:21:36PM +0200, Michał Pecio wrote: + u32 baseline, mantissa, exponent; Please keep these as unsigned int. What's the reason for that? u32 is the exact width needed to perform these computations, while unsigned int is something a bit unspecified. Because you don't need a specific size (e.g. for marshalling). You can safely assume an int is at least 32 bits. + /* Calculate and return the exact baud rate. */ + baud = (baseline / mantissa) (exponent 1); You should handle division by zero here. It cannot currently happen as we cap the baudrate, but you should handle it here nonetheless. You can still assume a non-zero baudrate, though. How about that? mantissa = baseline / baud; if (mantissa == 0) mantissa = 1; Writing zero to the chip as it's currently done probably doesn't make much sense anyway... Sure, that works. Thanks, Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
On Wed, Jul 15, 2015 at 01:35:58PM +0200, Michał Pecio wrote: This commit fixes the following issues: 1. The 9th bit of buf was believed to be the LSB of divisor's exponent, but the hardware interprets it as MSB (9th bit) of the mantissa. The exponent is actually one bit shorter and applies to base 4, not 2 as previously believed. 2. Loop iterations doubled the exponent instead of incrementing. 3. The exponent wasn't checked for overflow. 4. The function returned requested rate instead of actual rate. Due to issue #2, the old code deviated from the wrong formula described in #1 and actually yielded correct rates when divisor was lower than 4096 by using exponents of 0, 2 or 4 base-2, interpreted as 0, 1, 2 base-4 with the 9th mantissa bit clear. However, at 93.75 kbaud or less the rate turned out too slow due to #2 or too fast due to #2 and #3. I tested this patch by sending and validating 0x00,0x01,..,0xff to an FTDI dongle at 234, 987, 2401, 9601, 31415, 115199, 250k, 500k, 750k, 1M, 1.5M, 3M+1 baud. All rates passed. I also used pv to check speed at some rates unsupported by FTDI: 45 (the lowest possible), 2M, 4M, 5M and 6M-1. Looked sane. Signed-off-by: Michal Pecio michal.pe...@gmail.com Now applied with stable tag. Thanks for fixing this. Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] USB: pl2303: Rewrite pl2303_encode_baud_rate_divisor
This commit fixes the following issues:
1. The 9th bit of buf was believed to be the LSB of divisor's
exponent, but the hardware interprets it as MSB (9th bit) of the
mantissa. The exponent is actually one bit shorter and applies
to base 4, not 2 as previously believed.
2. Loop iterations doubled the exponent instead of incrementing.
3. The exponent wasn't checked for overflow.
4. The function returned requested rate instead of actual rate.
Due to issue #2, the old code deviated from the wrong formula
described in #1 and actually yielded correct rates when divisor
was lower than 4096 by using exponents of 0, 2 or 4 base-2,
interpreted as 0, 1, 2 base-4 with the 9th mantissa bit clear.
However, at 93.75 kbaud or less the rate turned out too slow
due to #2 or too fast due to #2 and #3.
I tested this patch by sending and validating 0x00,0x01,..,0xff
to an FTDI dongle at 234, 987, 2401, 9601, 31415, 115199, 250k,
500k, 750k, 1M, 1.5M, 3M+1 baud. All rates passed.
I also used pv to check speed at some rates unsupported by FTDI:
45 (the lowest possible), 2M, 4M, 5M and 6M-1. Looked sane.
Signed-off-by: Michal Pecio michal.pe...@gmail.com
---
drivers/usb/serial/pl2303.c | 32 +++-
1 file changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index f5257af..3f9a808 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -362,22 +362,36 @@ static speed_t pl2303_encode_baud_rate_direct(unsigned
char buf[4],
static speed_t pl2303_encode_baud_rate_divisor(unsigned char buf[4],
speed_t baud)
{
- unsigned int tmp;
+ u32 baseline, mantissa, exponent;
/*
* Apparently the formula is:
-* baudrate = 12M * 32 / (2^buf[1]) / buf[0]
+* baudrate = 12M * 32 / (mantissa * 4^exponent)
+* where
+* mantissa = buf[8:0]
+* exponent = buf[11:9]
*/
- tmp = 1200 * 32 / baud;
+ baseline = 1200 * 32;
+ mantissa = baseline / baud;
+ exponent = 0;
+ while (mantissa = 512) {
+ if (exponent 7) {
+ mantissa = 2; /* divide by 4 */
+ exponent++;
+ } else {
+ /* Exponent is maxed. Trim mantissa and leave. */
+ mantissa = 511;
+ break;
+ }
+ }
+
buf[3] = 0x80;
buf[2] = 0;
- buf[1] = (tmp = 256);
- while (tmp = 256) {
- tmp = 2;
- buf[1] = 1;
- }
- buf[0] = tmp;
+ buf[1] = exponent 1 | mantissa 8;
+ buf[0] = mantissa 0xff;
+ /* Calculate and return the exact baud rate. */
+ baud = (baseline / mantissa) (exponent 1);
return baud;
}
--
2.4.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
