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.

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

emaste@ Will you take this + MFC ?

Sure I can do that, might be tomorrow.

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.

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

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.

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?

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.

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.

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

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.

@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!