Add Proliic new chip: PL2303TB & PL2303N(G)

DISCUSSION Johan Hovold 1 participant 1 message Jan 8, 2019 USB

Share & tools

Johan Hovold

author

2019-01-08 17:01 UTC #0 lore raw

Quoted 4 lines · Charles Yeh

Add new PID to support PL2303TB (TYPE_HX)
Add new PID to support PL2303(N)GC/GB/GS/GT/GL/GE (TYPE_HXN) Add new Flow Control to support PL2303(N)GC/GB/GS/GT/GL/GE (TYPE_HXN) Add new Pull-Up Mode to support PL2303HXD (TYPE_HX)

So the above should go in three separate patches as we already discussed (PL2303TB PID, TYPE_HXN support + PIDs, "pull-up mode").

Quoted 1 line

Fixed warning:restricted__le16 degrades to integer

When you update and resend a patch, it's good to include changelog information like this, but please put it below the cut-off line (---) below so that it doesn't end up in the commit log.

Also include a patch revision in the Subject of the mail. Let's call this one v2, so next time use the following Subject prefix:

	[PATCH v3]: USB: serial: pl2303: add ...

(also after breaking the current patch into a series of three or more patches).

Quoted 116 lines

Signed-off-by Charles Yeh <[email protected]>

---
 drivers/usb/serial/pl2303.c | 116 +++++++++++++++++++++++++++++-------
 drivers/usb/serial/pl2303.h |  11 ++++
 2 files changed, 106 insertions(+), 21 deletions(-)

drivers/usb/serial/pl2303.c +39 -2

index a4e0d13fc121..8c71612e1811 100644
@@ -46,6 +46,13 @@ static const struct usb_device_id id_table[] = {
 	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_HCR331) },
 	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_MOTOROLA) },
 	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_ZTEK) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303_PRODUCT_ID_TB) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GC) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GB) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GT) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GL) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GE) },
+	{ USB_DEVICE(PL2303_VENDOR_ID, PL2303G_PRODUCT_ID_GS) },
 	{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID) },
 	{ USB_DEVICE(IODATA_VENDOR_ID, IODATA_PRODUCT_ID_RSAQ5) },
 	{ USB_DEVICE(ATEN_VENDOR_ID, ATEN_PRODUCT_ID),

@@ -123,9 +130,11 @@ MODULE_DEVICE_TABLE(usb, id_table);
 
 #define VENDOR_WRITE_REQUEST_TYPE	0x40
 #define VENDOR_WRITE_REQUEST		0x01
+#define VENDOR_WRITE_NREQUEST		0x80
 
 #define VENDOR_READ_REQUEST_TYPE	0xc0
 #define VENDOR_READ_REQUEST		0x01
+#define VENDOR_READ_NREQUEST		0x81
 
 #define UART_STATE_INDEX		8
 #define UART_STATE_MSR_MASK		0x8b

@@ -139,11 +148,21 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+#define TYPE_HX_READ_OTP_STATUS_REGISTER	0x8484
+#define TYPE_HX_EXTERNAL_PULLUP_MODE	0x08
+#define TYPE_HX_PULLUP_MODE_REG		0x09
+#define TYPE_HXN_UART_FLOWCONTROL	0x0A
+#define TYPE_HXN_HARDWAREFLOW		0xFA
+#define TYPE_HXN_SOFTWAREFLOW		0xEE
+#define TYPE_HXN_NOFLOW			0xFF
+#define TYPE_HXN_RESET_DOWN_UPSTREAM	0x07
+
 static void pl2303_set_break(struct usb_serial_port *port, bool enable);
 
 enum pl2303_type {
 	TYPE_01,	/* Type 0 and 1 (difference unknown) */
 	TYPE_HX,	/* HX version of the pl2303 chip */
+	TYPE_HXN,	/* HXN version of the pl2303 chip */
 	TYPE_COUNT
 };

@@ -173,16 +192,26 @@ static const struct pl2303_type_data pl2303_type_data[TYPE_COUNT] = {
 	[TYPE_HX] = {
 		.max_baud_rate =	12000000,
 	},
+	[TYPE_HXN] = {
+		.max_baud_rate =	12000000,
+	},
 };
 
 static int pl2303_vendor_read(struct usb_serial *serial, u16 value,
 							unsigned char buf[1])
 {
 	struct device *dev = &serial->interface->dev;
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
 	int res;
+	u8 request;
+
+	if (spriv->type == &pl2303_type_data[TYPE_HXN])
+		request = VENDOR_READ_NREQUEST;
+	else
+		request = VENDOR_READ_REQUEST;
 
 	res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
-			VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
+			request, VENDOR_READ_REQUEST_TYPE,
 			value, 0, buf, 1, 100);
 	if (res != 1) {
 		dev_err(dev, "%s - failed to read [%04x]: %d\n", __func__,

@@ -201,12 +230,19 @@ static int pl2303_vendor_read(struct usb_serial *serial, u16 value,
 static int pl2303_vendor_write(struct usb_serial *serial, u16 value, u16 index)
 {
 	struct device *dev = &serial->interface->dev;
+	struct pl2303_serial_private *spriv = usb_get_serial_data(serial);
 	int res;
+	u8 request;
 
 	dev_dbg(dev, "%s - [%04x] = %02x\n", __func__, value, index);
 
+	if (spriv->type == &pl2303_type_data[TYPE_HXN])
+		request = VENDOR_WRITE_NREQUEST;
+	else
+		request = VENDOR_WRITE_REQUEST;
+
 	res = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
-			VENDOR_WRITE_REQUEST, VENDOR_WRITE_REQUEST_TYPE,
+			request, VENDOR_WRITE_REQUEST_TYPE,
 			value, index, NULL, 0, 100);
 	if (res) {
 		dev_err(dev, "%s - failed to write [%04x]: %d\n", __func__,

@@ -286,6 +322,7 @@ static int pl2303_startup(struct usb_serial *serial)
 	struct pl2303_serial_private *spriv;
 	enum pl2303_type type = TYPE_01;
 	unsigned char *buf;
+	int res;
 
 	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
 	if (!spriv)

drivers/usb/serial/pl2303.c static int pl2303_startup(struct usb_serial *serial) · line 353

Quoted 10 lines

drivers/usb/serial/pl2303.c +7 -0

@@ -307,26 +344,36 @@ static int pl2303_startup(struct usb_serial *serial)
 		type = TYPE_01;		/* type 1 */
 	dev_dbg(&serial->interface->dev, "device type: %d\n", type);
 
+	if (type == TYPE_HX) {
+		res = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+			VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
+			TYPE_HX_READ_OTP_STATUS_REGISTER, 0, buf, 1, 100);
+		if (res != 1)
+			type = TYPE_HXN;
+	}

So HXN looks like an HX, but responds with an error when trying to read any (?) register using VENDOR_READ_REQUEST? Isn't there any more direct way of determining the device type?

My old HXD device report bcdUSB as 0x0110, and you tested you also tested against 0x0200 in a previous version of this patch. How does bcdUSB relate to the various types?

Note that you should use le16_to_cpu() to access bcdUSB safely also on big-endian systems (which the kbuild test robot somewhat cryptically reported).

Quoted 27 lines

drivers/usb/serial/pl2303.c +7 ~~-14~~

+
 	spriv->type = &pl2303_type_data[type];
 	spriv->quirks = (unsigned long)usb_get_serial_data(serial);
 	spriv->quirks |= spriv->type->quirks;
 
 	usb_set_serial_data(serial, spriv);
 
-	pl2303_vendor_read(serial, 0x8484, buf);
-	pl2303_vendor_write(serial, 0x0404, 0);
-	pl2303_vendor_read(serial, 0x8484, buf);
-	pl2303_vendor_read(serial, 0x8383, buf);
-	pl2303_vendor_read(serial, 0x8484, buf);
-	pl2303_vendor_write(serial, 0x0404, 1);
-	pl2303_vendor_read(serial, 0x8484, buf);
-	pl2303_vendor_read(serial, 0x8383, buf);
-	pl2303_vendor_write(serial, 0, 1);
-	pl2303_vendor_write(serial, 1, 0);
-	if (spriv->quirks & PL2303_QUIRK_LEGACY)
-		pl2303_vendor_write(serial, 2, 0x24);
-	else
-		pl2303_vendor_write(serial, 2, 0x44);
+	if (type != TYPE_HXN) {
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_write(serial, 0x0404, 0);
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_read(serial, 0x8383, buf);
+		pl2303_vendor_read(serial, 0x8484, buf);

drivers/usb/serial/pl2303.c static int pl2303_startup(struct usb_serial *serial) · line 376

Quoted 10 lines

drivers/usb/serial/pl2303.c +10 -0

+		pl2303_vendor_write(serial, 0x0404, 1);
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_read(serial, 0x8383, buf);
+		pl2303_vendor_write(serial, 0, 1);
+		pl2303_vendor_write(serial, 1, 0);
+		if (spriv->quirks & PL2303_QUIRK_LEGACY)
+			pl2303_vendor_write(serial, 2, 0x24);
+		else
+			pl2303_vendor_write(serial, 2, 0x44);
+	}

Is this even needed for pre-HXN devices, or could perhaps some of it just be removed? Can you explain what it does?

Quoted 20 lines

drivers/usb/serial/pl2303.c +7 -2

 	kfree(buf);

@@ -671,15 +718,37 @@ static void pl2303_set_termios(struct tty_struct *tty,
 	}
 
 	if (C_CRTSCTS(tty)) {
-		if (spriv->quirks & PL2303_QUIRK_LEGACY)
+		if (spriv->type == &pl2303_type_data[TYPE_01])
 			pl2303_vendor_write(serial, 0x0, 0x41);
+		else if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_UART_FLOWCONTROL,
+						 TYPE_HXN_HARDWAREFLOW);
 		else
 			pl2303_vendor_write(serial, 0x0, 0x61);
 	} else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) == 0x11 &&
 			STOP_CHAR(tty) == 0x13) {
-		pl2303_vendor_write(serial, 0x0, 0xc0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_UART_FLOWCONTROL,
+						 TYPE_HXN_SOFTWAREFLOW);

drivers/usb/serial/pl2303.c static void pl2303_set_termios(struct tty_struct *tty, · line 741

Quoted 10 lines

drivers/usb/serial/pl2303.c +8 -1

+		else
+			pl2303_vendor_write(serial, 0x0, 0xc0);
 	} else {
-		pl2303_vendor_write(serial, 0x0, 0x0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_UART_FLOWCONTROL,
+						 TYPE_HXN_NOFLOW);
+		else
+			pl2303_vendor_write(serial, 0x0, 0x0);
+	}

So this is becoming a bit hard to read. The idea with the type struct was to abstract the differences. We could add something like

	u8 uart_flowctrl_reg;
	u8 uart_flowctrl_hw;
	u8 uart_flowctrl_sw;
	u8 uart_flowctrl_none;

to struct pl2303_type_data, and replace the above with just three calls to pl2303_vendor_write().

If you do this, then do this as a preparatory patch before adding HXN support.

We should probably do something similar with the read and write requests instead of adding conditionals in those paths.

drivers/usb/serial/pl2303.c static void pl2303_set_termios(struct tty_struct *tty, · line 751

Quoted 10 lines

drivers/usb/serial/pl2303.c +10 -0

+
+	if (spriv->type == &pl2303_type_data[TYPE_HX]) {
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_write(serial, 0x0404, TYPE_HX_PULLUP_MODE_REG);
+		pl2303_vendor_read(serial, 0x8484, buf);
+		pl2303_vendor_read(serial, 0x8383, buf);
+		if ((u16)*buf & TYPE_HX_EXTERNAL_PULLUP_MODE) {
+			pl2303_vendor_write(serial, 0x0, 0x31);
+			pl2303_vendor_write(serial, 0x1, 0x01);
+		}

So this bit needs to go in it's own patch with a commit message explaining why it is needed. Don't forget to replace the "magic constants" with descriptive defines.

*buf is u8 and no need to cast to u16, as I also already pointed out in my comments to an earlier version of the patch.

Quoted 6 lines

drivers/usb/serial/pl2303.c +0 -0

 	}
 
 	kfree(buf);

@@ -720,8 +789,13 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
 		usb_clear_halt(serial->dev, port->read_urb->pipe);
 	} else {

drivers/usb/serial/pl2303.c static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port) · line 798

Quoted 10 lines

drivers/usb/serial/pl2303.c +7 -2

 		/* reset upstream data pipes */
-		pl2303_vendor_write(serial, 8, 0);
-		pl2303_vendor_write(serial, 9, 0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial,
+					TYPE_HXN_RESET_DOWN_UPSTREAM, 0);
+		else {
+			pl2303_vendor_write(serial, 8, 0);
+			pl2303_vendor_write(serial, 9, 0);
+		}

So this is a bit harder to abstract, but could be done using function pointers although it's probably not worth it just yet.

Just make sure to do add bracket ({}) also to the branch you add here.

Thanks,
Johan