Hello Martin,
are you planning sending v8 soon?
There seems to be general interest, if it makes working on it more joyful:
https://www.reddit.com/r/raspberry_pi/comments/1gznt7p/has_anyone_used_this_board_usb_to_8channel_serial/
Thank you
David
On 25/08/2024 06:56, Martin Blumenstingl wrote:
Hi Johan,
On Tue, Jul 23, 2024 at 6:13 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
[...]
Do you need to include any copyrights from the vendor driver? It looks
like at least some of the code below was copied from somewhere (but
perhaps not that much).
If you - or someone else - have any advice on this I'd be happy to go with that!
If you copy code directly (even if you clean it up after) you should
include it, but not necessarily if you just use it for reference for the
protocol.
It doesn't hurt mentioning anyway when you add the reference to the
vendor driver, for example:
Based on the XXX driver:
https://...
Copyright YYY
Thanks, I'll include that in the next version.
[...]
For slow speeds I never receive the "Transmitter holding register
empty" interrupt/signal when using the full TX buffer.
It's not that the interrupt/signal is late - it just never arrives.
I don't know why that is (whether the firmware tries to keep things
"fair" for other ports, ...) though.
Perhaps you can run some isolated experiments if you haven't already.
Submitting just a single URB with say 128, 512 or 1024 bytes of data and
see when/if you ever receive a transmitter holding empty interrupt.
How does the vendor driver handle this? Does it really wait for the THRE
interrupt before submitting more data?
The vendor driver:
- first acquires a per-device (not per port) write_lock [0]
- then waits for the (per-device, not per port) write buffer to be empty [1]
- and only then submits more data to be transmitted [2]
You could try increasing the buffer size to 2k and see how much is
received on the other end if you submit one URB (e.g. does the hardware
just drop the last 1k of data when the device fifo is full).
I have not tried this yet but if still relevant (after the info about
the THRE interrupt) then I can try it and share the results.
[...]
+ * If we ingest more data then usb_serial_generic_write() will
+ * internally try to process as much data as possible with any
+ * number of URBs without giving us the chance to wait in
+ * between transfers.
If the hardware really works this way, then perhaps you should not use
the generic write implementation. Just maintain a single urb per port
and don't submit it until the device fifo is empty.
I tried to avoid having to copy & paste (which then also means having
to maintain it down the line) most of the generic write
implementation.
This whole dance with waiting for the "Transmitter holding register
empty" by the way was the reason why parts of the transmit buffer got
lost, see the report from Nicolas in v6 [1]
I understand, but the generic implementation is not a good fit here as
it actively tries to make sure the device buffers are always full (e.g.
by using two URBs back-to-back).
If you can't find a way to make the hardware behave properly then a
custom implementation using a single URBs is preferable over trying
to limit the generic implementation like you did here. Perhaps bits can
be reused anyway (e.g. chars_in_buffer if you use the write fifo).
I cannot find any other usb-serial driver which uses this pattern.
Most devices seem to be happy to take more data once they trigger the
write_bulk_callback but not ch348.
If there's any other (even if it's not a usb-serial) driver that I can
use as a reference implementation for your suggestion?
I'm not sure whether to use a dedicated kthread, single threaded workqueue, ...
+static struct usb_serial_driver ch348_device = {
+ .driver = {
+ .owner = THIS_MODULE,
+ .name = "ch348",
+ },
+ .id_table = ch348_ids,
+ .num_ports = CH348_MAXPORT,
+ .num_bulk_in = 1,
+ .num_bulk_out = 1,
Set both of these to 2 so that core verifies that you have all four
endpoints.
I will have to test this because I thought that:
- using 2 here makes usb-serial allocate an URB as well and by default
assign it to the first and second port
- usb-serial should not touch the second bulk in / bulk out endpoint
(as they're entirely vendor / chip specific)
Setting these two should make core make sure that the endpoints exist,
and by default they will be assigned to the first and second port, but
you can override that calc_num_endpoints() (as you already do).
For the second IN EP, you could even let core allocate the URB and use
that instead of doing so manually (e.g. by submitting yourself or using
the generic read implementation as mxuport does).
Thanks for the hint - I have tried this and it indeed simplifies the code!
Best regards,
Martin
[0] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1100
[1] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1152-L1156
[2] https://github.com/WCHSoftGroup/ch9344ser_linux/blob/d4fc95fb1cca9962384ca88b0007df8738ae5829/driver/ch9344.c#L1166
--
David Heidelberg
