Message-ID: <CAFBinCCYsWHsNwi99kFqvLv+xOYtp9u3omhrPdV-hdH+5Cfyew@mail.gmail.com>
Date: Mon, 4 Aug 2025 23:35:35 +0200
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Johan Hovold <johan@...nel.org>
Cc: Corentin Labbe <clabbe@...libre.com>, gregkh@...uxfoundation.org, 
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org, david@...t.cz
Subject: Re: [PATCH v8 1/2] usb: serial: add support for CH348

On Mon, Aug 4, 2025 at 2:32 PM Johan Hovold <johan@...nel.org> wrote:
>
> On Tue, Jul 29, 2025 at 10:45:20PM +0200, Martin Blumenstingl wrote:
> > On Tue, Jul 29, 2025 at 11:43 AM Johan Hovold <johan@...nel.org> wrote:
> > > On Sat, Jul 26, 2025 at 04:54:17PM +0200, Martin Blumenstingl wrote:
>
> > > > I managed to get it to work now without any unnecessary waiting.
> > > > When I switched to just waiting for per-port THRE I accidentally
> > > > re-used the same URB (along with its buffer) for all ports. This of
> > > > course "corrupts" data, but it's my fault instead of the chip/firmware
> > > > causing it.
> > > > That's why I was referring to data corruption earlier.
> > > > Thanks for your persistence and for making me look at my code again
> > > > with a fresh mind.
> > >
> > > Glad to hear you got it working. Did you confirm that you really need to
> > > wait for THRE before submitting the next URB too? I don't see why the
> > > vendor driver would be doing this otherwise, but perhaps it only affects
> > > older, broken firmware, or something.
>
> > I'm using Corentin's test script [0] for sending data and by
> > connecting RX6 to TX7 and TX6 to RX7.
>
> May be better to use a second different device (rather than loopback)
> for testing so that you can separate any tx issues from rx issues.
I'll double-check it using a second device. The RX path has largely
been unmodified since the original submission, so it's likely that the
issue is indeed with the TX path.

> > For a 1024 byte buffer:
> > [ 3029.068311] ch348 ttyUSB6: submitted 509 bytes for urb 0
> > [ 3029.068827] ch348 ttyUSB6: submitted 509 bytes for urb 1
> > [ 3029.069363] ch348 ttyUSB7: submitted 5 bytes for urb 0
> > [ 3029.069902] ch348 ttyUSB7: UART_IIR_THRI - unknown byte: 0x00
> > [ 3029.215272] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > [ 3029.215908] ch348 ttyUSB6: submitted 6 bytes for urb 0
> > [ 3029.233628] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > -> data is received without corruption
> >
> > With a 2048 byte buffer the general flow seems fine:
> > [ 3031.073101] ch348 ttyUSB6: submitted 509 bytes for urb 0
> > [ 3031.073777] ch348 ttyUSB6: submitted 509 bytes for urb 1
> > [ 3031.220068] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > [ 3031.220697] ch348 ttyUSB6: submitted 509 bytes for urb 0
> > [ 3031.221342] ch348 ttyUSB6: submitted 509 bytes for urb 1
> > [ 3031.512113] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > [ 3031.512795] ch348 ttyUSB6: submitted 12 bytes for urb 0
> > [ 3031.513359] ch348 ttyUSB7: submitted 5 bytes for urb 0
> > [ 3031.513859] ch348 ttyUSB7: UART_IIR_THRI - unknown byte: 0x00
> > [ 3031.530476] ch348 ttyUSB6: UART_IIR_THRI - unknown byte: 0x00
> > However, the receiving end sees different data (at around byte 513-518
> > in my tests) than we wanted to send.
> >
> > My general flow is:
> > - check if we have received THRE - if not: don't transmit more data on this port
> > - submit up to two URBs with up to 512 - 3 (CH348_TX_HDRSIZE) bytes to
> > not exceed the HW TX FIFO size of 1024 bytes (page 1 in the datasheet)
> > if the kfifo has enough data
>
> If you're going to wait for the device fifo to clear completely you can
> just use a single urb with larger (1k) buffer too.
I set .bulk_out_size = 1024 in struct usb_serial_driver. Writing a 1k
buffer immediately results in:
   ch348 1-1:1.0: device disconnected

I don't know if I need to set some kind of flag on the URB to have it
split or whether the kernel / USB controller does that automatically
(as you can tell: I'm not familiar with USB).
If not: 512 byte transfers at a time it is.

> > If you want me to test something else then please let me know and I'll try it.
> > Otherwise I'll not dig much deeper, given the fact that I don't know
> > how the firmware works (e.g. in which order they send the status to
> > the host and what kind of state they hold internally) and we can still
> > optimize this later.
>
> Yeah, as long as you are certain that the generic implementation does
> not work and that you indeed need to track THRE per port.
I'll give it one more round in the next few days. If I can't get the
generic implementation to work then I'll call the current approach
good.

[...]
> > > > On my test board the CFG pin is HIGH. From how I understand you, RTS
> > > > should at least change (even if DTR is in TNOW mode).
> > > > No matter what I do: both pins are always LOW (right after modprobe,
> > > > after opening the console, closing the console again, ...).
> > > > I even set up the vendor driver to test this: it's the same situation there.
> > >
> > > I don't think the console code will assert DTR/RTS, you need to open the
> > > port as a regular tty.
>
> Yes, even if the device is configured in hardware for TNOW mode (instead
> of DTR function) you should still be able to control RTS (at least as
> long as the device is not configured for automatic hardware flow control).
I think I made it work, sort of.
It's a bit annoying because of code I don't understand. It seems that
R_4 has the following settings:
0x00 DTR off
0x01 DTR on
0x10 RTS off
0x11 RTS on
0x08 activate (used during port initialization)
0x50 HW flow on
0x51 no RTS / HW flow off

That said, poking 0x00, 0x01, 0x10 and 0x11 by themselves didn't do much.
One also has to write 0x06 to the per-port VEN_R register.
The vendor driver only does that in .set_termios, which I call
questionable until someone calls me out on this and is willing to
share a good reason why that's a good idea ;-)

However, I'm unable to control the RTS line of port 1. It works for
port 0, port 2 and 3 but not for port 1.
Ports 4-7 don't have the TNOW/DTR and RTS lines routed outside the
package, so I can't test these.


Best regards,
Martin

Powered by blists - more mailing lists