Confused about UART stop_tx() start_tx() functions.

[Grant Edwards <grant.b.edwards@xxxxxxxxx>]

I'm confused about the start_tx() and stop_tx() UART driver methods.

According to include/linux/serial_core.h

 * @stop_tx: ``void ()(struct uart_port *port)``
 *
 *      Stop transmitting characters. This might be due to the CTS line
 *      becoming inactive or the tty layer indicating we want to stop
 *      transmission due to an %XOFF character.
 *
 *      The driver should stop transmitting characters as soon as possible.
 *
 *      [...]
 *
 * @start_tx: ``void ()(struct uart_port *port)``
 *
 *      Start transmitting characters.
 *
 *      [...]

But in many drivers, that's not what those two functions do. For
example in rp2.c:

static void rp2_uart_stop_tx(struct uart_port *port)
{
        rp2_rmw_clr(port_to_up(port), RP2_TXRX_CTL, RP2_TXRX_CTL_TXIRQ_m);
}

static void rp2_uart_stop_rx(struct uart_port *port)
{
        rp2_rmw_clr(port_to_up(port), RP2_TXRX_CTL, RP2_TXRX_CTL_RXIRQ_m);
}

Those two functions *do not start/stop transmitting characters*, they
enable/disable the transmit interrupt. After disabling the transmit
interrupt, the UART continues to transmit anything in the tx FIFO (up
to 256 bytes). That hardly qualifies as "stop transmitting characters
as soon as possible".

Starting and stopping the transmission of characters is controlled by
the RP2_TXRX_CTL_TX_EN_m bit in that same register. Clearing that bit
will "stop transmitting characters as soon as possible" (at the end of
the byte currently in the tx shift register).

While stop/start_tx() behavior in rp2.c is clearly wrong according to
serial_core.h, it does seems to be what is expected by the serial core
code:

#define __uart_port_tx(uport, ch, flags, tx_ready, put_char, tx_done,         \
                       for_test, for_post)                                    \
({                                                                            \
        struct uart_port *__port = (uport);                                   \
        struct tty_port *__tport = &__port->state->port;                      \
        unsigned int pending;                                                 \
                                                                              \
        for (; (for_test) && (tx_ready); (for_post), __port->icount.tx++) {   \
             [write bytes into tx FIFO]                                       \
        }                                                                     \
                                                                              \
        [...]                                                                 \
                                                                              \
        pending = kfifo_len(&__tport->xmit_fifo);                             \
        if (pending < WAKEUP_CHARS) {                                         \
                uart_write_wakeup(__port);                                    \
                                                                              \
                if (!((flags) & UART_TX_NOSTOP) && pending == 0)              \
                        __port->ops->stop_tx(__port);                         \
        }                                                                     \
                                                                              \
        pending;                                                              \
})


It wouldn't make sense to copy data from the tx buffer into the tx
FIFO and then immediately call stop_tx() because the tx buffer is now
empty. Disabling the tx interrupt because the tx buffer is empty does
make sense.

However, there seem to be plenty of other drivers where stop_tx() and
start_tx() do actually start and stop the transmission of characters
as defined by serial_core.h.

21285.c:

#define tx_disable(port)        disable(port, tx_enabled_bit)

static void serial21285_stop_tx(struct uart_port *port)
{
        if (is_tx_enabled(port)) {
                disable_irq_nosync(IRQ_CONTX);
                tx_disable(port);
        }
}

Can somebody please clarify exactly what start_tx() and stop_tx() are
supposed to do?

Thanks

--
Grant