Message-ID: <Zp_WiocH4D14mEA7@hovoldconsulting.com>
Date: Tue, 23 Jul 2024 18:12:58 +0200
From: Johan Hovold <johan@...nel.org>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
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 1/1 v7] usb: serial: add support for CH348

On Mon, Jul 22, 2024 at 11:22:32PM +0200, Martin Blumenstingl wrote:

> On Mon, Jul 22, 2024 at 4:21 PM Johan Hovold <johan@...nel.org> wrote:
> > On Tue, May 07, 2024 at 01:15:22PM +0000, Corentin Labbe wrote:

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * USB serial driver for USB to Octal UARTs chip ch348.
> > > + *
> > > + * Copyright (C) 2022 Corentin Labbe <clabbe@...libre.com>
> >
> > 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

> > > +     u8 reg_iir;
> > > +     union {
> > > +             u8 lsr_signal;
> > > +             u8 modem_signal;
> > > +             u8 init_data[10];
> >
> > This init_data field is never used and looks a bit odd. What is it used
> > for?
> I think the firmware is sending back the configuration that was
> applied (see ch348_set_termios())
> 
> > You mentioned that that the modem lines are not yet used either.

> My understanding is: we neither use init_data nor modem_signal in the
> driver so let's drop them for now (and add them back when they're
> actually needed).

I think it's ok to keep, as reference. You do dump this data later even
if you don't currently use this union I noticed.

> > > +static int ch348_write(struct tty_struct *tty, struct usb_serial_port *port,
> > > +                    const unsigned char *buf, int count)
> > > +{
> > > +     struct ch348 *ch348 = usb_get_serial_data(port->serial);
> > > +     struct ch348_port *ch348_port = &ch348->ports[port->port_number];
> > > +     int ret, max_tx_size;
> > > +
> > > +     if (tty_get_baud_rate(tty) < 9600 && count >= 128)
> >
> > You don't hold the termios lock here so this needs to be handled
> > differently. Perhaps you can set a flag in set_termios if this is
> > really needed.
> >
> > > +             /*
> > > +              * Writing larger buffers can take longer than the hardware
> > > +              * allows before discarding the write buffer. Limit the
> > > +              * transfer size in such cases.
> > > +              * These values have been found by empirical testing.
> > > +              */
> > > +             max_tx_size = 128;
> >
> > Can you elaborate on you findings here? According to the vendor homepage
> > this device has a 1024 byte TX fifo per port. Are you really saying that
> > for some reason you can only fill it with 128 b when the line speed is
> > below 9600?

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

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).
 
> > > +     else
> > > +             /*
> > > +             * Only ingest as many bytes as we can transfer with one URB at
> > > +             * a time. Once an URB has been written we need to wait for the
> > > +             * R_II_B2 status event before we are allowed to send more data.
> >
> > As I mentioned above R_II_B2 appears to be the transfer holding register
> > empty flag in IIR. So you write one endpoint size worth of data and then
> > wait for all of it to be processed on the device before sending more.
> >
> > How big is the endpoint (please post lsusb -v)?

>        wMaxPacketSize     0x0200  1x 512 bytes

> > > +             * 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).

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

Johan

Powered by blists - more mailing lists