On Tue, Jul 19, 2022 at 09:20:58AM +0200, Johan Hovold wrote: > On Tue, Jul 19, 2022 at 04:04:53PM +0930, Jonathan Woithe wrote: > > On Tue, Jul 19, 2022 at 07:53:42AM +0200, Johan Hovold wrote: > > > > I noticed one register write which was removed during 4.10 and we also > > > added support for setting the LCR register which wasn't there before. > > > > > > Can you try the below patch which adds a register write that should > > > match what the old driver did at 115200 baud? > > > > I applied this patch to 672c0c5. With the patch as supplied there was no > > change: the first character was lost but all others were delivered as > > expected. > > > > > If that doesn't help, can you try just ifdeffing out the LCR update? > > > > I changed the second of the added "#if -0" directives to "#if 1" (thus > > removing the LCR update). This made things work: all characters were echoed > > back to minicom as they were pressed. > > Perfect, thanks for confirming. > > > As a final test I changed the first of the added "#if 1" directives to > > "#if 0", effectively removing the extra register write you added earlier. > > The resulting kernel still worked. It appears that this extra register > > write doesn't have any effect as far as the missing first character is > > concerned. > > Thanks for checking. > > > On the basis of this, it seems the LCR update causes the loss of the first > > character on version 0x27 hardware. > > Right, so it seems we have two issues here: > > First, the disabled rx timer, which we can fix. > > And second, the lost character after updating LCR, which we may be able > to work around, for example, by disabling LCR control for older devices > (i.e. pre-4.10 behaviour) or by not updating LCR unless the settings > have changed (but that would still likely cause a lost character when we > do). A third option may be to experiment with using the INIT command to > set the line-speed and LCR, but that also has some side-effects like > toggling the modem-control lines on termios changes, which I wanted to > avoid. > > And we still need to set LCR on probe, which may trigger the lost > character unless we do this using the INIT command and that turns out > to work. > > 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. Using the patch below, the device appears to function correctly: characters are echoed back to minicom as soon as they are sent and the first characater is not lost. Changing line speed in minicom does not result in a lost character either. 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. Regards jonathan > >From 1c937d58f83df00a7879ff82a2b980d11b3faf7b 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. > > Also try using the INIT command to update LCR to work around a separate > issue with lost characters after LCR updates on older devices. Note that > using INIT like this triggers a similar problem on newer CH341A. > > Reported-by: Jonathan Woithe <jwoithe@xxxxxxxxxx> > Link: https://lore.kernel.org/r/Ys1iPTfiZRWj2gXs@xxxxxxxxxxxxxxxxxxx > Not-Signed-off-yet-by: Johan Hovold <johan@xxxxxxxxxx> > Signed-off-by: Johan Hovold <johan@xxxxxxxxxx> > --- > drivers/usb/serial/ch341.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c > index 2798fca71261..d6a4b2572c9c 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,24 @@ 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. > + */ > + if (priv->version > 0x27) > + val |= BIT(7); > + > + /* > + * At least one device with version 0x27 loses characters after > + * updating LCR directly, try using INIT for that. > + * > + * Note that this appears to cause lost characters on newer CH341A. > */ > - val |= BIT(7); > + if (priv->version <= 0x27) { > + r = ch341_control_out(dev, CH341_REQ_SERIAL_INIT, > + lcr << 8 | 0x9c, val); > + return r; > + } > > r = ch341_control_out(dev, CH341_REQ_WRITE_REG, > CH341_REG_DIVISOR << 8 | CH341_REG_PRESCALER, > @@ -308,7 +327,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 --
