Hi Johan,
Thanks for you check the patch..
I will reply to you on the next Monday.
Because I am currently on a business trip in China (3/28~4/6)
Johan Hovold <johan@xxxxxxxxxx> 於 2019年4月2日 週二 下午3:22寫道：
>
> On Wed, Feb 13, 2019 at 08:30:00PM +0800, Charles Yeh wrote:
> > 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.
>
> > 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.
> >
> > 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.
>
> > Signed-off-by: Charles Yeh <charlesyeh522@xxxxxxxxx>
> > ---
> > drivers/usb/serial/pl2303.c | 131 +++++++++++++++++++++++++++++-------
> > drivers/usb/serial/pl2303.h | 7 ++
> > 2 files changed, 113 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> > index bb3f9aa4a909..d7d557e01390 100644
> > --- a/drivers/usb/serial/pl2303.c
> > +++ b/drivers/usb/serial/pl2303.c
> > @@ -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
> > @@ -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?
>
> > +#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)
> > @@ -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?
>
> > + 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?
>
> > + 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);
> > + 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?
>
> > 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?
>
> > + 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.
>
> > + 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);
> > + 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.
>
> > }
> >
> > 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.
>
> > + 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?
>
> > + 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.
>
> > + }
> > }
> >
> > /* Setup termios */
> > diff --git a/drivers/usb/serial/pl2303.h b/drivers/usb/serial/pl2303.h
> > index 559941ca884d..898ddc1a7302 100644
> > --- a/drivers/usb/serial/pl2303.h
> > +++ b/drivers/usb/serial/pl2303.h
> > @@ -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.
>
> >
> > #define ATEN_VENDOR_ID 0x0557
> > #define ATEN_VENDOR_ID2 0x0547
>
> Thanks,
> Johan
