On Tue, Jul 19, 2022 at 09:48:59PM +0930, Jonathan Woithe wrote: > On Tue, Jul 19, 2022 at 05:35:22PM +0930, Jonathan Woithe wrote: > > > Can you try the below patch and see if that still triggers the lost > > > character after probe and line settings changes? Please also try to > > > confirm that LCR can be updated this way at all. > > > > : > > Of course all testing to this point has been with a loopback adapter which > > means there's no indication whether line speed (or any other LCR parameters) > > have really changed. For complicated reasons I can't easily test using the > > programmer hardware right now. I will see if I can work out a way to verify > > speed and LCR changes with the hardware I have available. Hacking up a > > null-modem cable and connecting to a PC which happens to have a real > > hardware serial port may be the easiest. > > I have done further testing with the patch below in place. Unfortunately > the PC with the real serial port decided not to boot, so instead I'm testing > with a 3-wire null modem cable into a PL2303-based USB-serial converter on > another box running a 5.15.38 kernel. Flow control was turned off since the > cable wasn't wired for that. > > Tests done in minicom: > > * Speed: change from 115200 to 38400 and back again. > > The speed setting of the CH341 appears to work correctly. > > * Data length: change from 8 to 7 bits in minicom on both adapters > > CH341 = 8 bits, PL2303 = 8 bits > - Data delivered correctly in both directions. > > CH341 = 8 bits, PL2303 = 7 bits > - Data sent by the CH341 received correctly by the PL2303 > - Data sent by the PL2303 not received correctly by the CH341 > > CH341 = 7 bits, PL2303 = 7 bits > - Data sent by the CH341 received correctly by the PL2303 > - Data sent by the PL2303 not received correctly by the CH341 > > CH341 = 7 bits, PL2303 = 8 bits > - Data delivered correctly in both directions. > > There are too many variables in play to know what to make of this given > that the response of the PL2303 driver to these changes is unknown. At > first glance the results appear to be rather inconsistent. Maybe the > data length setting is not always affecting tx and rx on one or both of > the adapters. Thanks for testing this. The above observations appear consistent with LCR updates for your CH341 not taking effect (e.g. stuck at 8N1 unlike the PL2303). An easy way to test this is to send the letter 'q' (0x71) from CH341 to PL2303 (in 8N1 mode). This should be received as 0xf1 if the CH341 is in 7N1 mode as the stop bit is interpreted as MSB, or as 0x71 if still in 8N1 mode. > I'm not sure what this proves: most of these settings don't appear to be > having any effect on either adapter. In the end, 8N1 works and this is, > after all, what most people would be using. The situation with the patch is > clearly better than it was without the patch because the CH341 is now > delivering data in a timely manner - just like it did under 4.4.14. > > The behaviour of the LCR setting on version 0x27 may not be a new thing: all > my applications use 8N1, so if any of the other combinations were faulty > under 4.4.14 I wouldn't know. I may look into testing the LCR settings > under 4.4.14 if you think there's value in it. I expect it will show that > the patch below restores the 4.4.14 behaviour for hardware version 0x27, and > that the LCR issues were present in 4.4.14 but masked due to the almost > universal use of 8N1). I can also try to work out an alternative way to > get a real hardware serial port if that would help: I'm thinking it would > remove any doubt about the LCR setting at one end of the link. Actually, LCR configuration wasn't supported before 4.10 either so the only question would be if LCR control works at all with your chip (except for the lost character). I found this thread where Micahel provides some details after apparently having disassembled the vendor driver: https://lore.kernel.org/all/2e80916d-1be8-dc0f-abf9-adc0feea1803@xxxxxxxxxxxxxxx/ Based on that (and the comment that made it into the driver), chips before version 0x30 uses a different protocol for updating LCR so that anything but 8N1 has indeed likely never worked for these chips. Could you try the patch below, which simply disables LCR updates for older chips and which I believe you already confirmed works. And then as a follow up, see if enabling the LCR update again has any effect on the word size (e.g. rerun the test you did above, or the "q" test I mentioned). This may give an indication of how far we are from being able to support LCR updates on older chips even this is not something we need to implement now. Johan >From 4d12bb8ba4fe760cd5893b7c5f981524e16536fc Mon Sep 17 00:00:00 2001 From: Johan Hovold <johan@xxxxxxxxxx> Date: Mon, 18 Jul 2022 10:21:41 +0200 Subject: [PATCH] USB: serial: ch341: fix disabled rx timer on older devices At least one older CH341 appears to have the RX timer enable bit inverted so that setting it disables the RX timer and prevents the FIFO from emptying until it is full. Only set the RX timer enable bit for devices with version newer than 0x27 (even though this probably affects all pre-0x30 devices). Also disable LCR updates for pre-0x30 devices which use a different protocol and where the current register write causes the next received character to be lost. FIXME: split in two patches Reported-by: Jonathan Woithe <jwoithe@xxxxxxxxxx> Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@xxxxxxxxxxxxxxxxxxx Fixes: 4e46c410e050 ("USB: serial: ch341: reinitialize chip on reconfiguration") Fixes: 55fa15b5987d ("USB: serial: ch341: fix baud rate and line-control handling") Not-Signed-off-yet-by: Johan Hovold <johan@xxxxxxxxxx> --- drivers/usb/serial/ch341.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c index 2798fca71261..af01a462cc43 100644 --- a/drivers/usb/serial/ch341.c +++ b/drivers/usb/serial/ch341.c @@ -97,7 +97,10 @@ struct ch341_private { u8 mcr; u8 msr; u8 lcr; + unsigned long quirks; + u8 version; + unsigned long break_end; }; @@ -250,8 +253,12 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev, /* * CH341A buffers data until a full endpoint-size packet (32 bytes) * has been received unless bit 7 is set. + * + * At least one device with version 0x27 appears to have this bit + * inverted. */ - val |= BIT(7); + if (priv->version > 0x27) + val |= BIT(7); r = ch341_control_out(dev, CH341_REQ_WRITE_REG, CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER, @@ -265,6 +272,9 @@ static int ch341_set_baudrate_lcr(struct usb_device *dev, * (stop bits, parity and word length). Version 0x30 and above use * CH341_REG_LCR only and CH341_REG_LCR2 is always set to zero. */ + if (priv->version < 0x30) + return 0; + r = ch341_control_out(dev, CH341_REQ_WRITE_REG, CH341_REG_LCR2 << 8 | CH341_REG_LCR, lcr); if (r) @@ -308,7 +318,9 @@ static int ch341_configure(struct usb_device *dev, struct ch341_private *priv) r = ch341_control_in(dev, CH341_REQ_READ_VERSION, 0, 0, buffer, size); if (r) return r; - dev_dbg(&dev->dev, "Chip version: 0x%02x\n", buffer[0]); + + priv->version = buffer[0]; + dev_dbg(&dev->dev, "Chip version: 0x%02x\n", priv->version); r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, 0, 0); if (r < 0) -- 2.35.1
