On Mon, 24 Jun 2024, Filip Štědronský wrote: > Hi, > > it seems that the EM485 mode on Designware UART controller is broken. > At the start of transmission, the RTS line is correctly asserted, but > it never gets deasserted. > > Checked this with a logic analyzer. See: > > https://regnarg.cz/tmp/em485_bad.sr (PulseView dump) > https://regnarg.cz/tmp/em485_bad.png > > The test system is FriendlyElec NanoPi Neo-LTS (Allwinner H3). > For reference, the test userspace program I used: > > https://regnarg.cz/tmp/485.c > > The mechanism seems to be this: > > - The DW UART driver sets UART_CAP_NOTEMT when creating the port > (8250_dwlib.c:dw8250_setup_port) to indicate that the controller > does not generate interrupt on emptying the shift register. > > - This should be then used in 8250_port.c:__stop_tx to use a timer > instead of an interrupt to trigger DE deassertion. > > - However, the port also goes through the 8250 autoconfig mechanism. > For my controller, dw8250_setup_port does _not_ set UPF_FIXED_TYPE, > so it tries to autodetect port type. As part of this, the > up->capabilities field is reset, dropping the UART_CAP_NOTEMT > (8250_port.c:autoconfig). > * On this particular controller, the Component Version Register > (DW_UART_UCV) returns zero, so the dw8250_setup_port function > returns after this block: > > reg = dw8250_readl_ext(p, DW_UART_UCV); > return; > > - Without UART_CAP_NOTEMT, __stop_tx does not set up a timer and instead > leaves deasserting DE to an interrupt that never comes. > > If I hotfix autoconfig to preserve UART_CAP_NOTEMT, the > EM485 functionality seems to work correctly: > > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index 893bc493f662..1c2d24074722 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -1140,6 +1140,7 @@ static void autoconfig(struct uart_8250_port *up) > struct uart_port *port = &up->port; > unsigned long flags; > unsigned int old_capabilities; > + unsigned int preserved_capabilities; > > if (!port->iobase && !port->mapbase && !port->membase) > return; > @@ -1155,7 +1156,8 @@ static void autoconfig(struct uart_8250_port *up) > */ > uart_port_lock_irqsave(port, &flags); > > - up->capabilities = 0; > + preserved_capabilities = up->capabilities & UART_CAP_NOTEMT; > + up->capabilities = preserved_capabilities; > up->bugs = 0; > > if (!(port->flags & UPF_BUGGY_UART)) { > @@ -1266,7 +1268,7 @@ static void autoconfig(struct uart_8250_port *up) > > port->fifosize = uart_config[up->port.type].fifo_size; > old_capabilities = up->capabilities; > - up->capabilities = uart_config[port->type].flags; > + up->capabilities = uart_config[port->type].flags | preserved_capabilities; > up->tx_loadsz = uart_config[port->type].tx_loadsz; > > if (port->type == PORT_UNKNOWN) > > And the result: > > https://regnarg.cz/tmp/em485_good.sr (PulseView dump) > https://regnarg.cz/tmp/em485_good.png > > But I am not quite sure what the 'proper' fix is, as I am missing a lot > of context regarding the relationship between DW UART, 8250 and the > various port types. While this patch fixes the problem (and I'm not strictly against it since this is kind of the minimal fix), it would be good to discuss about the fundamental problem behind this, the current autoconfig code is pretty all-encompassing. To me it looks like in this case dw8250 code would want to tell to the autoconfig "please config x, and y, etc. but don't config z". I wonder if it would be good to start moving towards direction where it's not either-or like UPF_FIXED_TYPE currently is and driver would have finer-grained control over what it wants to get autoconfigured. If somebody else has some vision on what the autoconfig code should look like, please chim in. -- i.
