uplcom: support new chip revision PL2303HXN.

[tomli_tomli.me]

Summary

Recently, a new chip revision of the PL2303 USB-to-serial converter, PL2303HXN, appeared on the market. This family includes 6 variants, PL2303GC (0x23a3), PL2303GB (0x23b3), PL2303GT (0x23c3), PL2303GL (0x23d3), PL2303GE (0x23e3), and PL2303GS (0x23f3).

This commit adds support of these new chips. Changes includes:

0. Overall, the new PL2303HXN is very similar to PL2303HX. Very little change is needed.

1. PL2303HXN uses a different bRequest, UPLCOM_SET_REQUEST_PL2303HXN (0x80) to set registers, the previous UPLCOM_SET_REQUEST (0x01) cannot be used. Coincidentally, this fact allows us to distinguish PL2303HXN from PL2303HX by issuing an old-style UPLCOM_SET_REQUEST on a status register and checking for an error.

2. PL2303HXN uses different registers to set and clear RTS/CTS.

3. The previous initialization sequences, uplcom_reset() and uplcom_pl2303_init() are no longer needed.

4. In addition, PL2303HXN uses a different command to perform the "upstream data pipe" reset.

5. Like PL2303HX, PL2303HXN supports baud rates up to 12Mb.

Code changes in this commit were obtained from straight from OpenBSD's uplcom.c with almost no modification, the list of chip names and USB PIDs was obtained from Linux.

This is my first patch to FreeBSD. I have no idea on how to pick a reviewer, so I just selected two based on the historical commit logs. Apologize if it's not appropriate.

Test Plan

Initial test: after applying this patch, a basic test shows that the uplcom(4) can identify the new chip and successfully established serial communication with an embedded device.

RTS/CTS set/clear test: using the original patch, once the RTS/CTS is enable, communication with the target (which doesn't support RTS/CTS) is lost and cannot even be restored by disabling it. The updated patch fixes the problem.

[• hselasky]

Changes look good. Do you have a reference to the OpenBSD patches?

[tomli_tomli.me]

Remove an accidental empty line change. Recreate patch with context "9999".

[tomli_tomli.me]

In D27952#624330, @hselasky wrote:

Changes look good. Do you have a reference to the OpenBSD patches?

Yes, please see

https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/dev/usb/uplcom.c.diff?r1=1.75&r2=1.76&f=h

It's basically a 99% identical patch, with only two minor differences.

• I changed the macro names to make the coding style "fits" in FreeBSD, as the uplcom driver in three BSDs have diverged since then (e.g. instead of "UPLCOM_HXN_SET_CRTSCTS", I used "UPLCOM_SET_CRTSCTS_PL2303HXN").

• Instead of use

	if (sc->sc_type != UPLCOM_TYPE_HXN) {
		err = uplcom_reset(sc);
                ...
        }

I used

static usb_error_t
uplcom_reset(struct uplcom_softc *sc, struct usb_device *udev)
{
        ...
        if (sc->sc_chiptype == TYPE_PL2303HXN) {
                /* PL2303HXN doesn't need this reset sequence */
                return (0);
        }
        ...
}

If you think the original style is cleaner, I'd happy to submit a new diff.

[tomli_tomli.me]

Hold on, the empty line change in line 883 is still there, I accidentally submitted the original patch again ;(... Let me fix that first.

[tomli_tomli.me]

Really recreate patch with context. Also fix the accidental and useless empty line change.

[• hselasky]

emaste@ Will you take this + MFC ?

[emaste]

emaste@ Will you take this + MFC ?

Sure I can do that, might be tomorrow.

[tomli_tomli.me]

This is embarrassing, but do not commit the current version of this patch yet. I just did a final reread of the code and found a bug.

if (t->c_cflag & CRTSCTS) {
...
} else {
	req.bmRequestType = UT_WRITE_VENDOR_DEVICE;
	req.bRequest = UPLCOM_SET_REQUEST;
	USETW(req.wValue, 0);
	USETW(req.wIndex, 0);
	USETW(req.wLength, 0);
	ucom_cfg_do_request(sc->sc_udev, &sc->sc_ucom, 
				&req, NULL, 0, 1000);
}

The else branch of the RTS/CTS code still uses UPLCOM_SET_REQUEST and hasn't been converted for PL2303HXN. Sure enough, my testing showed that once RTS/CTS is activated, it could never be turned off again. I'm now submitting a fixed patch for your review, thanks for your patience.

[tomli_tomli.me]

New patch uploaded. This one fixes the RTS/CTS bug.

[• hselasky]

Let me know when you have finished testing!

[tomli_tomli.me]

I already did a basic go/no-go test before submitting the revised patch. Now you've said that, just to be extra careful, I'm going to verify the waveforms on my oscilloscope for all supported baud rates and report back ASAP.

[emaste]

Code changes in this commit were obtained from straight from OpenBSD's uplcom.c

Is there an additional copyright statement in OpenBSD's uplcom.c that we should bring over?

[tomli_tomli.me]

I just completed the tests.

Conclusion: all tests were successful.

• Chip Detection Test: Plugged an old AVR firmware programmer powered by the PL2303HX chip to my system with my patch, read debug printf. It was correctly identified as 2303HX, not 2303HXN. And my new RS-232 converter with PL2303HXN was also correctly identified. Conclusion: The HXN chip detection logic is working and I'm not breaking the driver for previous users, good.

• Baud rate test: Connected Pin 3 (TXD) to the oscilloscope, wrote a quick script to send data at different baud rate by sending the letter capital "U" with 8N1, and watch the frequency of the square wave on the oscilloscope. I tested 12000000, 6000000, 3000000, 2457600, 1228800, 921600, 806400, 614400, 460800, 403200, 268800, 256000, 230400, 201600, 161280, 134400, 128000, 115200, 57600, 56000, 38400, 28800, 19200, 14400, 9600, 7200, 4800, 3600, 2400, 1800, 1200, 600, 300, 150, 110, and 75 baud.
  
  Almost all baud rates worked perfectly. Only two had problems: 256000 (period is 1130 us, not the expected 78.125 us) and 110 (526 us, not 18181 us). But those two aren't supported rates in uplcom_rates[] and the existing comments in the code already explicitly mentioned that 256000 doesn't work regardless of chip revision. I also double-checked on Linux and found those two baud rates are broken as well. this is a pre-existing condition and possibly a corner case related to the undocumented baud rate configuration, not the problem of my patch. Good.

• RTS/CTS test - Wrote a script to send "U" at 9600 baud, 8N1, with switchable RTS/CTS setting. Observed waveform on the oscilloscope. When RTS/CTS is off, free-running square wave can be seen, when it's on, the waveform disappears. Switching it off again, waveform reappears. Also, when RTS/CTS is on, bridging Pin 7 (RTS) and Pin 8 (CTS) with a breadboard jumper wire causes the waveform to appear immediately. Good.

• data/stop/parity test - tested 5/6/7/8 data bits, 1/1.5/2 stop bits, none/odd/even parity. Verified on the oscilloscope.

[tomli_tomli.me]

In D27952#625379, @emaste wrote:

Code changes in this commit were obtained from straight from OpenBSD's uplcom.c

Is there an additional copyright statement in OpenBSD's uplcom.c that we should bring over?

OpenBSD's uplcom.c only contains a copyright statement from NetBSD Foundation, just like ours.

[emaste]

In D27952#625456, @tomli_tomli.me wrote:

OpenBSD's uplcom.c only contains a copyright statement from NetBSD Foundation, just like ours.

Thanks for checking, I have added it to my staging tree here: https://github.com/emaste/freebsd/commit/6c5133085fb4b48222d8e29fc087e49a5e33b818

[tomli_tomli.me]

Thanks, that was fast! I only started using FreeBSD since 24 days ago, I never expected to get something merged into the upstream within 2 days.

[• hselasky]

@emaste: I had some spare time and pushed the patch.

@tomli_tomli.me :

Thank you for contributing to FreeBSD. If you have more USB patches let us know.

Your patch was very trivial to review. Else getting patches in might take some more time!