Skip to contentSkip to conversation

USB:serial:pl2303:Add new PID to support PL2303HXN (TYPE_HXN)

DISCUSSION Johan Hovold 1 participant 1 message Apr 2, 2019 USB

lore
Share & tools
lore

Johan Hovold

author
#0 lore raw
Quoted 1 line · Charles Yeh

Prolific has developed a new USB to UART chip: PL2303HXN (PL2303GC/PL2303GS/PL2303GT/PL2303GL/PL2303GE)

Ok, let's get back to this one.

First a general comment; please make sure to address all review comments. I've already pointed out some issues that still hasn't been fixed and I've asked questions that have gone unanswered.

If you disagree on something then just say so, but ignoring feedback is just going to make this take longer than necessary.

For a start, please fix up the Subject line as we already discussed, and make sure to wrap your commit messages at 72 columns or so (I've reflown the rest of the message below).

Always include a changelog (below the cut-off line) when resending so we know what changed when you update your patches.

Quoted 26 lines

The Vendor request used by the PL2303HXN (TYPE_HXN) is different from the existing PL2303 series (TYPE_HX & TYPE_01).
Therefore, different Vendor requests are used to issue related commands.

1. Added a new TYPE_HXN type in pl2303_type_data, and then executes new Vendor request, new flow control and other related instructions if TYPE_HXN is recognized.

2. Because the new PL2303HXN can only accept the new Vendor request, the old Vendor request cannot be accepted (the error message will be returned) So first determine the TYPE_HX or TYPE_HXN through TYPE_HX_READ_STATUS_REG in pl2303_startup.

2.1 If the return message is "1", then the PL2303 is the existing TYPE_HX/ TYPE_01 series. The other settings in pl2303_startup are to continue execution.

2.2 If the return message is "not 1", then the PL2303 is the new TYPE_HXN series. The other settings in pl2303_startup are ignored. (PL2303HXN will directly use the default value in the hardware, no need to add additional settings through the software)

3. In pl2303_open: Because TYPE_HXN is different from the instruction of down/up stream used by TYPE_HX. Therefore, we will also execute different instructions here.

Quoted 7 lines

4. In pl2303_set_termios: The UART flow control instructions used by TYPE_HXN/TYPE_HX/TYPE_01 are different. Therefore, we will also execute different instructions here.

5. In pl2303_vendor_read & pl2303_vendor_write, since TYPE_HXN is different from the vendor request instruction used by TYPE_HX/TYPE_01, it will also execute different instructions here.

That's a good summary of the differences, but on a more general level, can you confirm the following:

1. The HXN register layout is entirely different from HX and earlier devices.

2. HXN use the same CDC class requests (line encoding, etc) as earlier revisions.

Can you send me documentation for the HXN protocol? That would help a lot in finding the right abstraction level for this.

Quoted 36 lines
Signed-off-by Charles Yeh <[email protected]>
---
 drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
 drivers/usb/serial/pl2303.h |   7 ++
 2 files changed, 113 insertions(+), 25 deletions(-)
drivers/usb/serial/pl2303.c +8 -0
index bb3f9aa4a909..d7d557e01390 100644
@@ -47,6 +47,12 @@ static const struct usb_device_id id_table[] = {
 	{ 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),

@@ -129,9 +135,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

drivers/usb/serial/pl2303.c MODULE_DEVICE_TABLE(usb, id_table); · line 161

Quoted 9 lines
drivers/usb/serial/pl2303.c +5 -0
@@ -145,11 +153,30 @@ MODULE_DEVICE_TABLE(usb, id_table);
 #define UART_OVERRUN_ERROR		0x40
 #define UART_CTS			0x80
 
+#define TYPE_HX_READ_STATUS_REG		0x8080
+#define TYPE_HXN_FLOWCONTROL_REG	0x0A
 
+#define TYPE_HXN_HARDWAREFLOW_DATA	0xFA
+#define TYPE_HXN_SOFTWAREFLOW_DATA	0xEE
+#define TYPE_HXN_NOFLOW_DATA		0xFF

What exactly does bits 0x15 (bits 4, 2 and 0) do?

Is register 0x0a really only used for flow control?

Quoted 80 lines
drivers/usb/serial/pl2303.c +34 -2
+#define TYPE_HX_01_FLOWCONTROL_REG	0x00
+#define TYPE_01_HARDWAREFLOW_DATA	0x41
+#define TYPE_HX_HARDWAREFLOW_DATA	0x61
+#define TYPE_HX_01_SOFTWAREFLOW_DATA	0xC0
+#define TYPE_HX_01_NOFLOW_DATA		0x00
+#define UART_XON_CHAR			0x11
+#define UART_XOFF_CHAR			0x13
+#define HX_RESET_DOWN_UPSTREAM_REG1	0x08
+#define HX_RESET_DOWN_UPSTREAM_REG2	0x09
+#define HX_RESET_DOWN_UPSTREAM_DATA	0x00
+#define HXN_RESET_DOWN_UPSTREAM_REG	0x07
+#define HXN_RESET_DOWN_UPSTREAM_DATA	0x00
+
 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
 };

@@ -179,16 +206,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__,

@@ -207,12 +244,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__,

@@ -292,6 +336,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 361

Quoted 4 lines
drivers/usb/serial/pl2303.c +1 -0
@@ -313,26 +358,37 @@ 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) {

In an earlier version of your patch, you also checked bcdUSB here. Why did you remove it?

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

Quoted 4 lines
drivers/usb/serial/pl2303.c +4 -0
+		res = usb_control_msg(serial->dev,
+			usb_rcvctrlpipe(serial->dev, 0),
+			VENDOR_READ_REQUEST, VENDOR_READ_REQUEST_TYPE,
+			TYPE_HX_READ_STATUS_REG, 0, buf, 1, 100);

Please name the registers after what they do, not how you use them. What is register 0 that you read here? Does it have a name?

Quoted 30 lines
drivers/usb/serial/pl2303.c +10 -14
+		if (res != 1)
+			type = TYPE_HXN;
+	}
+
 	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 391

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);
+	}

Fair enough, since the HXN doesn't use the same registers, this needs to be done only for HXN or earlier, but we should probably add an initialisation callback instead of spreading conditionals throughout the driver.

I think I know roughly what the code above does now, but since you are the only ones with access to the documentation, perhaps you can explain why TYPE_01 sets bit 0x20 of register 2 instead of 0x40 as the HX does? Is that even correct?

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

Quoted 8 lines
drivers/usb/serial/pl2303.c +0 -2
 	kfree(buf);
 

@@ -677,15 +733,30 @@ static void pl2303_set_termios(struct tty_struct *tty,
 	}
 
 	if (C_CRTSCTS(tty)) {
-		if (spriv->quirks & PL2303_QUIRK_LEGACY)
-			pl2303_vendor_write(serial, 0x0, 0x41);

Why do the TYPE_01 not set bit 0x20 here? Do the legacy device support both auto-rts and auto-cts?

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

Quoted 11 lines
drivers/usb/serial/pl2303.c +11 -0
+		if (spriv->type == &pl2303_type_data[TYPE_01])
+			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
+						TYPE_01_HARDWAREFLOW_DATA);
+		else if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
+						TYPE_HXN_HARDWAREFLOW_DATA);
+		else
+			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
+						TYPE_HX_HARDWAREFLOW_DATA);
+	} else if (I_IXON(tty) && !I_IXANY(tty) && START_CHAR(tty) ==
+			UART_XON_CHAR && STOP_CHAR(tty) == UART_XOFF_CHAR) {

No need to add defines for the start and stop char here.

Quoted 8 lines
drivers/usb/serial/pl2303.c +3 -4
+		if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
+						TYPE_HXN_SOFTWAREFLOW_DATA);
 		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);

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

Quoted 10 lines
drivers/usb/serial/pl2303.c +8 -1
+			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
+						TYPE_HX_01_SOFTWAREFLOW_DATA);
 	} else {
-		pl2303_vendor_write(serial, 0x0, 0x0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN])
+			pl2303_vendor_write(serial, TYPE_HXN_FLOWCONTROL_REG,
+						TYPE_HXN_NOFLOW_DATA);
+		else
+			pl2303_vendor_write(serial, TYPE_HX_01_FLOWCONTROL_REG,
+						TYPE_HX_01_NOFLOW_DATA);

As already mentioned, the above is hardly readable. When studying the current driver, I noticed a couple of bugs that I'm preparing fixes for.

Specifically, we shouldn't be overwriting the entire control register, which changes the tranceiver suspend mode. Are there similar problems with not doing bit updates of register 0x0a?

Either way, rebasing your patches on top of those should allow this to be cleaned up somewhat.

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

Quoted 10 lines
drivers/usb/serial/pl2303.c +1 -2
 	}
 
 	kfree(buf);

@@ -726,8 +797,18 @@ static int pl2303_open(struct tty_struct *tty, struct usb_serial_port *port)
 		usb_clear_halt(serial->dev, port->read_urb->pipe);
 	} else {
 		/* reset upstream data pipes */
-		pl2303_vendor_write(serial, 8, 0);
-		pl2303_vendor_write(serial, 9, 0);
+		if (spriv->type == &pl2303_type_data[TYPE_HXN])

You need to added braces ({}) to both branches here.

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

Quoted 3 lines
drivers/usb/serial/pl2303.c +3 -0
+			pl2303_vendor_write(serial,
+					HXN_RESET_DOWN_UPSTREAM_REG,
+					HXN_RESET_DOWN_UPSTREAM_DATA);

Can you write anything to this register to reset the buffers, or does it have to be 0?

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

Quoted 7 lines
drivers/usb/serial/pl2303.c +7 -0
+		else {
+			pl2303_vendor_write(serial,
+					HX_RESET_DOWN_UPSTREAM_REG1,
+					HX_RESET_DOWN_UPSTREAM_DATA);
+			pl2303_vendor_write(serial,
+					HX_RESET_DOWN_UPSTREAM_REG2,
+					HX_RESET_DOWN_UPSTREAM_DATA);

I assume the older versions allow for the and up and down buffers to be reset independently? Please name these registers accordingly.

Quoted 9 lines
drivers/usb/serial/pl2303.c +1 -0
+		}
 	}
 
 	/* Setup termios */
drivers/usb/serial/pl2303.h +0 -0
index 559941ca884d..898ddc1a7302 100644

drivers/usb/serial/pl2303.h line 30

Quoted 10 lines
drivers/usb/serial/pl2303.h +7 -0
@@ -21,6 +21,13 @@
 #define PL2303_PRODUCT_ID_MOTOROLA	0x0307
 #define PL2303_PRODUCT_ID_ZTEK		0xe1f1
 
+/* PL2303HXN , TYPE_HXN */
+#define PL2303G_PRODUCT_ID_GC	0x23A3
+#define PL2303G_PRODUCT_ID_GB	0x23B3
+#define PL2303G_PRODUCT_ID_GT	0x23C3
+#define PL2303G_PRODUCT_ID_GL	0x23D3
+#define PL2303G_PRODUCT_ID_GE	0x23E3
+#define PL2303G_PRODUCT_ID_GS	0x23F3

Just use PL2303_ as prefix.

drivers/usb/serial/pl2303.h line 32

Quoted 3 lines
drivers/usb/serial/pl2303.h +0 -0
 #define ATEN_VENDOR_ID		0x0557
 #define ATEN_VENDOR_ID2		0x0547

Thanks,
Johan