[PATCH] USB: serial: cp210x: Improve baudrate support for CP2102N

[PATCH] USB: serial: cp210x: Improve baudrate support for CP2102N

[Karoly Pados]

The CP2102N supports more baudrates than earlier chips by SiLabs.
This patch adds support for all rates documented in the datasheet
of this device.

Signed-off-by: Karoly Pados <pados@pados.hu>
---
 drivers/usb/serial/cp210x.c | 39 ++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index b1849f657e01..793b86252c46 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -357,6 +357,9 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CP210X_PARTNUM_CP2104	0x04
 #define CP210X_PARTNUM_CP2105	0x05
 #define CP210X_PARTNUM_CP2108	0x08
+#define CP210X_PARTNUM_CP2102N_QFN28	0x20
+#define CP210X_PARTNUM_CP2102N_QFN24	0x21
+#define CP210X_PARTNUM_CP2102N_QFN20	0x22
 #define CP210X_PARTNUM_UNKNOWN	0xFF
 
 /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
@@ -758,8 +761,12 @@ static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *ctl)
 /*
  * cp210x_quantise_baudrate
  * Quantises the baud rate as per AN205 Table 1
+ * The CP2102N is fully (except for baud rate aliasing) software-
+ * compatible, but supports some additional baudrates. However, there is
+ * no quantitisation table available for this model, so in this case we
+ * take the supported baudrate which is closest to the requested one.
  */
-static unsigned int cp210x_quantise_baudrate(unsigned int baud)
+static unsigned int cp210x_quantise_baudrate(unsigned int baud, bool cp2102n)
 {
 	if (baud <= 300)
 		baud = 300;
@@ -790,10 +797,17 @@ static unsigned int cp210x_quantise_baudrate(unsigned int baud)
 	else if (baud <= 491520)   baud = 460800;
 	else if (baud <= 567138)   baud = 500000;
 	else if (baud <= 670254)   baud = 576000;
-	else if (baud < 1000000)
-		baud = 921600;
-	else if (baud > 2000000)
-		baud = 2000000;
+	else if (cp2102n) {
+		if (baud <= 960800)        baud = 921600;
+		else if (baud <= 1100000)  baud = 1000000;
+		else if (baud <= 1350000)  baud = 1200000;
+		else if (baud <= 1750000)  baud = 1500000;
+		else if (baud <= 2500000)  baud = 2000000;
+		else                       baud = 3000000;
+	} else {
+		if (baud < 1000000)        baud = 921600;
+		else if (baud > 2000000)   baud = 2000000;
+	}
 	return baud;
 }
 
@@ -1045,16 +1059,19 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 static void cp210x_change_speed(struct tty_struct *tty,
 		struct usb_serial_port *port, struct ktermios *old_termios)
 {
-	u32 baud;
-
-	baud = tty->termios.c_ospeed;
+	bool is_cp2102n;
+	u32 baud = tty->termios.c_ospeed;
+	struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
 
-	/* This maps the requested rate to a rate valid on cp2102 or cp2103,
-	 * or to an arbitrary rate in [1M,2M].
+	/* This maps the requested rate to a rate valid on cp2102(n) or
+	 * cp2103 or to an arbitrary rate in [1M,2M].
 	 *
 	 * NOTE: B0 is not implemented.
 	 */
-	baud = cp210x_quantise_baudrate(baud);
+	is_cp2102n =	(priv->partnum == CP210X_PARTNUM_CP2102N_QFN28) ||
+			(priv->partnum == CP210X_PARTNUM_CP2102N_QFN24) ||
+			(priv->partnum == CP210X_PARTNUM_CP2102N_QFN20);
+	baud = cp210x_quantise_baudrate(baud, is_cp2102n);
 
 	dev_dbg(&port->dev, "%s - setting baud rate to %u\n", __func__, baud);
 	if (cp210x_write_u32_reg(port, CP210X_SET_BAUDRATE, baud)) {
-- 
2.17.1

Re: [PATCH] USB: serial: cp210x: Improve baudrate support for CP2102N

[Johan Hovold]

On Fri, Jun 15, 2018 at 11:29:57PM +0200, Karoly Pados wrote:
> The CP2102N supports more baudrates than earlier chips by SiLabs.
> This patch adds support for all rates documented in the datasheet
> of this device.
> 
> Signed-off-by: Karoly Pados <pados@pados.hu>
> ---
>  drivers/usb/serial/cp210x.c | 39 ++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index b1849f657e01..793b86252c46 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -357,6 +357,9 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CP210X_PARTNUM_CP2104	0x04
>  #define CP210X_PARTNUM_CP2105	0x05
>  #define CP210X_PARTNUM_CP2108	0x08
> +#define CP210X_PARTNUM_CP2102N_QFN28	0x20
> +#define CP210X_PARTNUM_CP2102N_QFN24	0x21
> +#define CP210X_PARTNUM_CP2102N_QFN20	0x22
>  #define CP210X_PARTNUM_UNKNOWN	0xFF
>  
>  /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
> @@ -758,8 +761,12 @@ static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *ctl)
>  /*
>   * cp210x_quantise_baudrate
>   * Quantises the baud rate as per AN205 Table 1
> + * The CP2102N is fully (except for baud rate aliasing) software-
> + * compatible, but supports some additional baudrates. However, there is
> + * no quantitisation table available for this model, so in this case we
> + * take the supported baudrate which is closest to the requested one.
>   */
> -static unsigned int cp210x_quantise_baudrate(unsigned int baud)
> +static unsigned int cp210x_quantise_baudrate(unsigned int baud, bool cp2102n)

Pass in a struct usb_serial (or port) as a first argument instead which
allows for more readable code as well as for this to be reused to handle
other device type differences (e.g. only 2108 besides 2102n handles
baudrates over 921.6k).

>  {
>  	if (baud <= 300)
>  		baud = 300;
> @@ -790,10 +797,17 @@ static unsigned int cp210x_quantise_baudrate(unsigned int baud)
>  	else if (baud <= 491520)   baud = 460800;
>  	else if (baud <= 567138)   baud = 500000;
>  	else if (baud <= 670254)   baud = 576000;
> -	else if (baud < 1000000)
> -		baud = 921600;
> -	else if (baud > 2000000)
> -		baud = 2000000;
> +	else if (cp2102n) {

Add a static helper (looks like you add a define in the gpio patch)
cp210x_is_cp2102n(serial) here. You can even test for bit 0x20 in the
helper if you prefer (we can always change that later if needed).

> +		if (baud <= 960800)        baud = 921600;
> +		else if (baud <= 1100000)  baud = 1000000;
> +		else if (baud <= 1350000)  baud = 1200000;
> +		else if (baud <= 1750000)  baud = 1500000;
> +		else if (baud <= 2500000)  baud = 2000000;
> +		else                       baud = 3000000;

And even if the current code uses this odd formatting, your amendments
should not.

> +	} else {
> +		if (baud < 1000000)        baud = 921600;
> +		else if (baud > 2000000)   baud = 2000000;
> +	}
>  	return baud;
>  }
>  
> @@ -1045,16 +1059,19 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>  static void cp210x_change_speed(struct tty_struct *tty,
>  		struct usb_serial_port *port, struct ktermios *old_termios)
>  {
> -	u32 baud;
> -
> -	baud = tty->termios.c_ospeed;
> +	bool is_cp2102n;
> +	u32 baud = tty->termios.c_ospeed;
> +	struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
>  
> -	/* This maps the requested rate to a rate valid on cp2102 or cp2103,
> -	 * or to an arbitrary rate in [1M,2M].
> +	/* This maps the requested rate to a rate valid on cp2102(n) or
> +	 * cp2103 or to an arbitrary rate in [1M,2M].
>  	 *
>  	 * NOTE: B0 is not implemented.
>  	 */
> -	baud = cp210x_quantise_baudrate(baud);
> +	is_cp2102n =	(priv->partnum == CP210X_PARTNUM_CP2102N_QFN28) ||
> +			(priv->partnum == CP210X_PARTNUM_CP2102N_QFN24) ||
> +			(priv->partnum == CP210X_PARTNUM_CP2102N_QFN20);
> +	baud = cp210x_quantise_baudrate(baud, is_cp2102n);

So most of these changes would not be needed. Just pass in port->serial
to cp210x_quantise_baudrate().

Thanks,
Johan

Re: [PATCH] USB: serial: cp210x: Improve baudrate support for CP2102N

[Karoly Pados]

Hello,

> Pass in a struct usb_serial (or port) as a first argument instead which
> allows for more readable code as well as for this to be reused to handle
> other device type differences (e.g. only 2108 besides 2102n handles
> baudrates over 921.6k).
> 

Sure, will do.

> Add a static helper (looks like you add a define in the gpio patch)
> cp210x_is_cp2102n(serial) here.

Yes I have macro for that in the GPIO patch, and I will turn that into a
static function too.

To keep the baudrate and gpio patches independent,
do you think it is a good idea if I make a new patch which only adds the
partnum defines and the helper function first, then baudrate v2 and gpio v2
can build onto it?

> You can even test for bit 0x20 in the
> helper if you prefer (we can always change that later if needed).
>

If you wish, but personally I think that is asking for future bugs
in the long run. Even though the helper can be easily adjusted if needed,
when/if a new partnum shows up which has nothing to do with the cp2102n,
no one will think of having to adjust cp2102n-spacific code until bug reports
start coming in. So I'd prefer to explicitly check for the packages, but in
the end I'll use whatever you prefer.

What do you prefer?

> And even if the current code uses this odd formatting, your amendments
> should not.
> 

Of course. I also saw this is odd, but (apparently wrongly) decided to 
stay consistent inside the function with existing code. I will change
that too.

Greetings,
Karoly


June 19, 2018 11:16 AM, "Johan Hovold" <johan@kernel.org> wrote:

> On Fri, Jun 15, 2018 at 11:29:57PM +0200, Karoly Pados wrote:
> 
>> The CP2102N supports more baudrates than earlier chips by SiLabs.
>> This patch adds support for all rates documented in the datasheet
>> of this device.
>> 
>> Signed-off-by: Karoly Pados <pados@pados.hu>
>> ---
>> drivers/usb/serial/cp210x.c | 39 ++++++++++++++++++++++++++-----------
>> 1 file changed, 28 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
>> index b1849f657e01..793b86252c46 100644
>> --- a/drivers/usb/serial/cp210x.c
>> +++ b/drivers/usb/serial/cp210x.c
>> @@ -357,6 +357,9 @@ static struct usb_serial_driver * const serial_drivers[] = {
>> #define CP210X_PARTNUM_CP2104 0x04
>> #define CP210X_PARTNUM_CP2105 0x05
>> #define CP210X_PARTNUM_CP2108 0x08
>> +#define CP210X_PARTNUM_CP2102N_QFN28 0x20
>> +#define CP210X_PARTNUM_CP2102N_QFN24 0x21
>> +#define CP210X_PARTNUM_CP2102N_QFN20 0x22
>> #define CP210X_PARTNUM_UNKNOWN 0xFF
>> 
>> /* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
>> @@ -758,8 +761,12 @@ static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *ctl)
>> /*
>> * cp210x_quantise_baudrate
>> * Quantises the baud rate as per AN205 Table 1
>> + * The CP2102N is fully (except for baud rate aliasing) software-
>> + * compatible, but supports some additional baudrates. However, there is
>> + * no quantitisation table available for this model, so in this case we
>> + * take the supported baudrate which is closest to the requested one.
>> */
>> -static unsigned int cp210x_quantise_baudrate(unsigned int baud)
>> +static unsigned int cp210x_quantise_baudrate(unsigned int baud, bool cp2102n)
> 
> Pass in a struct usb_serial (or port) as a first argument instead which
> allows for more readable code as well as for this to be reused to handle
> other device type differences (e.g. only 2108 besides 2102n handles
> baudrates over 921.6k).
> 
>> {
>> if (baud <= 300)
>> baud = 300;
>> @@ -790,10 +797,17 @@ static unsigned int cp210x_quantise_baudrate(unsigned int baud)
>> else if (baud <= 491520) baud = 460800;
>> else if (baud <= 567138) baud = 500000;
>> else if (baud <= 670254) baud = 576000;
>> - else if (baud < 1000000)
>> - baud = 921600;
>> - else if (baud > 2000000)
>> - baud = 2000000;
>> + else if (cp2102n) {
> 
> Add a static helper (looks like you add a define in the gpio patch)
> cp210x_is_cp2102n(serial) here. You can even test for bit 0x20 in the
> helper if you prefer (we can always change that later if needed).
> 
>> + if (baud <= 960800) baud = 921600;
>> + else if (baud <= 1100000) baud = 1000000;
>> + else if (baud <= 1350000) baud = 1200000;
>> + else if (baud <= 1750000) baud = 1500000;
>> + else if (baud <= 2500000) baud = 2000000;
>> + else baud = 3000000;
> 
> And even if the current code uses this odd formatting, your amendments
> should not.
> 
>> + } else {
>> + if (baud < 1000000) baud = 921600;
>> + else if (baud > 2000000) baud = 2000000;
>> + }
>> return baud;
>> }
>> 
>> @@ -1045,16 +1059,19 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
>> static void cp210x_change_speed(struct tty_struct *tty,
>> struct usb_serial_port *port, struct ktermios *old_termios)
>> {
>> - u32 baud;
>> -
>> - baud = tty->termios.c_ospeed;
>> + bool is_cp2102n;
>> + u32 baud = tty->termios.c_ospeed;
>> + struct cp210x_serial_private *priv = usb_get_serial_data(port->serial);
>> 
>> - /* This maps the requested rate to a rate valid on cp2102 or cp2103,
>> - * or to an arbitrary rate in [1M,2M].
>> + /* This maps the requested rate to a rate valid on cp2102(n) or
>> + * cp2103 or to an arbitrary rate in [1M,2M].
>> *
>> * NOTE: B0 is not implemented.
>> */
>> - baud = cp210x_quantise_baudrate(baud);
>> + is_cp2102n = (priv->partnum == CP210X_PARTNUM_CP2102N_QFN28) ||
>> + (priv->partnum == CP210X_PARTNUM_CP2102N_QFN24) ||
>> + (priv->partnum == CP210X_PARTNUM_CP2102N_QFN20);
>> + baud = cp210x_quantise_baudrate(baud, is_cp2102n);
> 
> So most of these changes would not be needed. Just pass in port->serial
> to cp210x_quantise_baudrate().
> 
> Thanks,
> Johan

Re: [PATCH] USB: serial: cp210x: Improve baudrate support for CP2102N

[Johan Hovold]

On Tue, Jun 19, 2018 at 09:50:54AM +0000, Karoly Pados wrote:
> Hello,
> 
> > Pass in a struct usb_serial (or port) as a first argument instead which
> > allows for more readable code as well as for this to be reused to handle
> > other device type differences (e.g. only 2108 besides 2102n handles
> > baudrates over 921.6k).
> > 
> 
> Sure, will do.
> 
> > Add a static helper (looks like you add a define in the gpio patch)
> > cp210x_is_cp2102n(serial) here.
> 
> Yes I have macro for that in the GPIO patch, and I will turn that into a
> static function too.
> 
> To keep the baudrate and gpio patches independent,
> do you think it is a good idea if I make a new patch which only adds the
> partnum defines and the helper function first, then baudrate v2 and gpio v2
> can build onto it?

No, that's fine. And you can submit it as a series, where the first
patch using the helper includes it (e.g. the baud rate one).

> > You can even test for bit 0x20 in the
> > helper if you prefer (we can always change that later if needed).
> 
> If you wish, but personally I think that is asking for future bugs
> in the long run. Even though the helper can be easily adjusted if needed,
> when/if a new partnum shows up which has nothing to do with the cp2102n,
> no one will think of having to adjust cp2102n-spacific code until bug reports
> start coming in. So I'd prefer to explicitly check for the packages, but in
> the end I'll use whatever you prefer.
> 
> What do you prefer?

Sure, I have no strong preference. You can keep the explicit package type
enumeration if you want, but move it to a static helper. We'll see how
this ends up being used in the series.

> > And even if the current code uses this odd formatting, your amendments
> > should not.
> 
> Of course. I also saw this is odd, but (apparently wrongly) decided to 
> stay consistent inside the function with existing code. I will change
> that too.

Yeah, that's admittedly a bit arbitrary. I should probably just clean
this up somehow once and for all.

By the way, have you tried setting other baudrates except the ones you
explicitly allow for here? According to the data sheet more rates should
be available, so perhaps just handling cp2102n as cp2108 (e.g. by not
trying to report back the exact rate used) or by actually calculating
the resulting rate could be another option?

Can be done later of course, just curious if you tried it.

Thanks,
Johan

Re: [PATCH] USB: serial: cp210x: Improve baudrate support for CP2102N

[Karoly Pados]

> By the way, have you tried setting other baudrates except the ones you
> explicitly allow for here? According to the data sheet more rates should
> be available, so perhaps just handling cp2102n as cp2108 (e.g. by not
> trying to report back the exact rate used) or by actually calculating
> the resulting rate could be another option?
> 
> Can be done later of course, just curious if you tried it.
> 

Yeah I know, I was thinking about this too while developing the patch. 
Officially the cp2102 and the cp2102n are fully software compatible (aside
from baudrate aliasing), but if the cp2102n chooses different baudrates for 
the same inputs than the older devices would then they couldn't/wouldn't be
compatible. So I concluded it must also be doing the quantisation.

Maybe I am too naive and trust the datasheet to much. I'll do some measurements
with my scope and let you know the results.

Best,
Karoly

June 20, 2018 11:39 AM, "Johan Hovold" <johan@kernel.org> wrote:

> On Tue, Jun 19, 2018 at 09:50:54AM +0000, Karoly Pados wrote:
> 
>> Hello,
>> 
>> Pass in a struct usb_serial (or port) as a first argument instead which
>> allows for more readable code as well as for this to be reused to handle
>> other device type differences (e.g. only 2108 besides 2102n handles
>> baudrates over 921.6k).
>> 
>> Sure, will do.
>> 
>> Add a static helper (looks like you add a define in the gpio patch)
>> cp210x_is_cp2102n(serial) here.
>> 
>> Yes I have macro for that in the GPIO patch, and I will turn that into a
>> static function too.
>> 
>> To keep the baudrate and gpio patches independent,
>> do you think it is a good idea if I make a new patch which only adds the
>> partnum defines and the helper function first, then baudrate v2 and gpio v2
>> can build onto it?
> 
> No, that's fine. And you can submit it as a series, where the first
> patch using the helper includes it (e.g. the baud rate one).
> 
>> You can even test for bit 0x20 in the
>> helper if you prefer (we can always change that later if needed).
>> 
>> If you wish, but personally I think that is asking for future bugs
>> in the long run. Even though the helper can be easily adjusted if needed,
>> when/if a new partnum shows up which has nothing to do with the cp2102n,
>> no one will think of having to adjust cp2102n-spacific code until bug reports
>> start coming in. So I'd prefer to explicitly check for the packages, but in
>> the end I'll use whatever you prefer.
>> 
>> What do you prefer?
> 
> Sure, I have no strong preference. You can keep the explicit package type
> enumeration if you want, but move it to a static helper. We'll see how
> this ends up being used in the series.
> 
>> And even if the current code uses this odd formatting, your amendments
>> should not.
>> 
>> Of course. I also saw this is odd, but (apparently wrongly) decided to
>> stay consistent inside the function with existing code. I will change
>> that too.
> 
> Yeah, that's admittedly a bit arbitrary. I should probably just clean
> this up somehow once and for all.
> 
> By the way, have you tried setting other baudrates except the ones you
> explicitly allow for here? According to the data sheet more rates should
> be available, so perhaps just handling cp2102n as cp2108 (e.g. by not
> trying to report back the exact rate used) or by actually calculating
> the resulting rate could be another option?
> 
> Can be done later of course, just curious if you tried it.
> 
> Thanks,
> Johan

Re: [PATCH] USB: serial: cp210x: Improve baudrate support for CP2102N

[Johan Hovold]

On Wed, Jun 20, 2018 at 09:51:41AM +0000, Karoly Pados wrote:
> > By the way, have you tried setting other baudrates except the ones you
> > explicitly allow for here? According to the data sheet more rates should
> > be available, so perhaps just handling cp2102n as cp2108 (e.g. by not
> > trying to report back the exact rate used) or by actually calculating
> > the resulting rate could be another option?
> > 
> > Can be done later of course, just curious if you tried it.
> 
> Yeah I know, I was thinking about this too while developing the patch. 
> Officially the cp2102 and the cp2102n are fully software compatible (aside
> from baudrate aliasing), but if the cp2102n chooses different baudrates for 
> the same inputs than the older devices would then they couldn't/wouldn't be
> compatible. So I concluded it must also be doing the quantisation.

Yeah, that's probably right, but the older devices do not support
rates > 1 Mbaud so that logic does not necessarily apply there.

> Maybe I am too naive and trust the datasheet to much. I'll do some
> measurements with my scope and let you know the results.

Cool. We can keep the old behaviour for < 1Mbaud, but it would be nice
to know if you can generate rates other than the 4-5 +1Mbauds rates that
were explicitly mentioned in the data sheet.

Thanks,
Johan