On Fri 2024-09-13 16:11:35, John Ogness wrote:
> Move IER handling out of rs485_start_tx() callback and into a new
> wrapper serial8250_rs485_start_tx(). Replace all callback call sites
> with wrapper, except for the console write() callback, where it is
> inappropriate to modify IER.
Sigh, I am trying to review this patch but I am not familiar with the
code. Feel free to ignore me when the questions are completely off.
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1370,7 +1370,6 @@ static void serial8250_stop_rx(struct uart_port *port)
> serial8250_rpm_get(up);
>
> up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
> - up->port.read_status_mask &= ~UART_LSR_DR;
> serial_port_out(port, UART_IER, up->ier);
>
> serial8250_rpm_put(up);
> @@ -1543,16 +1542,20 @@ static inline void __start_tx(struct uart_port *port)
> *
> * Generic callback usable by 8250 uart drivers to start rs485 transmission.
> * Assumes that setting the RTS bit in the MCR register means RTS is high.
> - * (Some chips use inverse semantics.) Further assumes that reception is
> - * stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the
> - * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
> + * (Some chips use inverse semantics.)
> + * It does not disable RX interrupts. Use the wrapper function
> + * serial8250_rs485_start_tx() if that is also needed.
> */
> void serial8250_em485_start_tx(struct uart_8250_port *up)
> {
> unsigned char mcr = serial8250_in_MCR(up);
>
> + /*
> + * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
> + * disabled, so explicitly mask it.
> + */
> if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> - serial8250_stop_rx(&up->port);
> + up->port.read_status_mask &= ~UART_LSR_DR;
This change is related to disabling UART_IER_RDI but we do not longer
disable it in this code path.
Why do we need to do it here, please?
Why is it needed only in the em485-specific path, please?
I tried to understand the code and am in doubts:
On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
so seems to be relater.
But the "Some chips set..." comment has been added by the commit
058bc104f7ca5c83d81 ("serial: 8250: Generalize rs485 software emulation").
And I do not see any explanation why it was added in this code path
even though UART_LSR_DR and UART_IER_RDI were manipulated in
serial8250_stop_rx() which can be called also in other code
paths via uport->ops->stop_rx().
Also the comment suggests that this fixes a bug in some chips but
the line has been added into 1.1.60 back in 2007.
--- a/drivers/char/ChangeLog
+++ b/drivers/char/ChangeLog
@@ -1,3 +1,28 @@
+Sat Oct 29 18:17:34 1994 Theodore Y. Ts'o (tytso@rt-11)
+
+ * serial.c (rs_ioctl, get_lsr_info): Added patch suggested by Arne
+ Riiber so that user mode programs can tell when the
+ transmitter shift register is empty.
+
+Thu Oct 27 23:14:29 1994 Theodore Y. Ts'o (tytso@rt-11)
+
+ * tty_ioctl.c (wait_until_sent): Added debugging printk statements
+ (under the #ifdef TTY_DEBUG_WAIT_UNTL_SENT)
+
+ * serial.c (rs_interrupt, rs_interrupt_single, receive_chars,
+ change_speed, rs_close): rs_close now disables receiver
+ interrupts when closing the serial port. This allows the
+ serial port to close quickly when Linux and a modem (or a
+ mouse) are engaged in an echo war; when closing the serial
+ port, we now first stop listening to incoming characters,
+ and *then* wait for the transmit buffer to drain.
+
+ In order to make this change, the info->read_status_mask
+ is now used to control what bits of the line status
+ register are looked at in the interrupt routine in all
+ cases; previously it was only used in receive_chars to
+ select a few of the status bits.
+
--- a/drivers/char/serial.c
+++ b/drivers/char/serial.c
[...]
@@ -1780,6 +1830,15 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
info->normal_termios = *tty->termios;
if (info->flags & ASYNC_CALLOUT_ACTIVE)
info->callout_termios = *tty->termios;
+ /*
+ * At this point we stop accepting input. To do this, we
+ * disable the receive line status interrupts, and tell the
+ * interrut driver to stop checking the data ready bit in the
+ * line status register.
+ */
+ info->IER &= ~UART_IER_RLSI;
+ serial_out(info, UART_IER, info->IER);
+ info->read_status_mask &= ~UART_LSR_DR;
if (info->flags & ASYNC_INITIALIZED) {
wait_until_sent(tty, 3000); /* 30 seconds timeout */
/*
=> It looks like it was not a fix for a "buggy chips". It looks
like it was part of the design.
>
> if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
> mcr |= UART_MCR_RTS;
> @@ -1562,6 +1565,18 @@ void serial8250_em485_start_tx(struct uart_8250_port *up)
> }
> EXPORT_SYMBOL_GPL(serial8250_em485_start_tx);
>
> +/**
> + * serial8250_rs485_start_tx() - stop rs485 reception, enable transmission
> + * @up: uart 8250 port
> + */
> +void serial8250_rs485_start_tx(struct uart_8250_port *up)
> +{
> + if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> + serial8250_stop_rx(&up->port);
> +
> + up->rs485_start_tx(up);
> +}
> +
> /* Returns false, if start_tx_timer was setup to defer TX start */
> static bool start_tx_rs485(struct uart_port *port)
> {
> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
> if (em485->tx_stopped) {
> em485->tx_stopped = false;
>
> - up->rs485_start_tx(up);
> + serial8250_rs485_start_tx(up);
If I get this correctly then this keeps the existing behavior when
up->rs485_start_tx == serial8250_em485_start_tx
Is this always the case, please?
The callback has been added by the commit 058bc104f7ca5c83d81
("serial: 8250: Generalize rs485 software emulation") because
8250_bcm2835aux.c driver needed to do something else.
Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
Will it still work as expected?
>
> if (up->port.rs485.delay_rts_before_send > 0) {
> em485->active_timer = &em485->start_tx_timer;
Best Regards,
Petr
