Message-ID: <CAFBinCBZhjs7DGEgxhz54Dg8aW3NX9_LdnoZeUZpm5ohaT_-oQ@mail.gmail.com>
Date: Tue, 29 Jul 2025 22:45:20 +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

Hi Johan,

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:
> > On Fri, Jul 25, 2025 at 12:07 PM Johan Hovold <johan@...nel.org> wrote:
>
> > > No, the vendor driver tracks THRE per port
> > > (ttyport[portnum].write_empty) and allows writing to more than one port
> > > in parallel (e.g. releases the device write_lock while waiting for the
> > > transfer to complete).
> > >
> > > I thought the problem was that you could not submit another urb for the
> > > *same* port until the device buffer for that port had been emptied?
> > >
> > > This seems to be what the vendor driver is preventing.
>
> > 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.

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 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.

[...]
> > The datasheet has a special note about DTR/TNOW (on page 8 for the CFG pin):
> > > Unified configuration: During power-on, if the CFG pin is
> > > at a high level or not connected, all DTRx/ TNOWx pins
> > > are configured to function as TNOW. CFG pin is low, all
> > > DTRx/ TNOWx pins are configured for DTR function.
>
> Got a link to the datasheet? Not sure what they refer to as TNOW.
Sure, try the direct link [1] - and if it doesn't work try [2].
It doesn't document any registers, so it's a high-level datasheet -
nor a programmers handbook.

> > 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.
>
> > If we need to make the DTR and RTS output something then the only way
> > I know of right now is to switch them to GPIO mode (I have code for
> > this but it's another ~300 lines patch on top of this).
>
> That should not be needed. It looked like the vendor driver had some
> variant of the usual request to control the modem lines. That should be
> all that is needed.
>
> Look at the vendor driver implementation of ch9344_tty_tiocmset() and
> ch9344_port_dtr_rts().
Thank you for the pointers. For today I -ETIMEDOUT, I'll take a look
at these in the next few days.


Best regards,
Martin


[0] https://github.com/montjoie/lava-tests/blob/5d4d83f2f71c37432dcdcc55ce3e31e74d133737/test2a2.py
[1] https://www.wch-ic.com/download/file?id=300
[2] https://www.wch-ic.com/downloads/CH348DS1_PDF.html

Powered by blists - more mailing lists