On Tue, 4 Jun 2024, Douglas Anderson wrote:
> The qcom_geni_serial_poll_bit() is supposed to be able to be used to
> poll a bit that's will become set when a TX transfer finishes. Because
> of this it tries to set its timeout based on how long the UART will
> take to shift out all of the queued bytes. There are two problems
> here:
> 1. There appears to be a hidden extra word on the firmware side which
> is the word that the firmware has already taken out of the FIFO and
> is currently shifting out. We need to account for this.
> 2. The timeout calculation was assuming that it would only need 8 bits
> on the wire to shift out 1 byte. This isn't true. Typically 10 bits
> are used (8 data bits, 1 start and 1 stop bit), but as much as 13
> bits could be used (14 if we allowed 9 bits per byte, which we
> don't).
>
> The too-short timeout was seen causing problems in a future patch
> which more properly waited for bytes to transfer out of the UART
> before cancelling.
>
> Rather than fix the calculation, replace it with the core-provided
> uart_fifo_timeout() function.
>
> NOTE: during earlycon, uart_fifo_timeout() has the same limitations
> about not being able to figure out the exact timeout that the old
> function did. Luckily uart_fifo_timeout() returns the same default
> timeout of 20ms in this case. We'll add a comment about it, though, to
> make it more obvious what's happening.
>
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
> Changes in v3:
> - Use uart_fifo_timeout() for timeout.
>
> Changes in v2:
> - New
>
> drivers/tty/serial/qcom_geni_serial.c | 37 +++++++++++++--------------
> 1 file changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 2bd25afe0d92..a48a15c2555e 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -124,7 +124,6 @@ struct qcom_geni_serial_port {
> dma_addr_t tx_dma_addr;
> dma_addr_t rx_dma_addr;
> bool setup;
> - unsigned int baud;
> unsigned long clk_rate;
> void *rx_buf;
> u32 loopback;
> @@ -269,24 +268,25 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> int offset, int field, bool set)
> {
> u32 reg;
> - struct qcom_geni_serial_port *port;
> - unsigned int baud;
> - unsigned int fifo_bits;
> - unsigned long timeout_us = 20000;
> - struct qcom_geni_private_data *private_data = uport->private_data;
> + unsigned long timeout_us;
>
> - if (private_data->drv) {
> - port = to_dev_port(uport);
> - baud = port->baud;
> - if (!baud)
> - baud = 115200;
> - fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
> - /*
> - * Total polling iterations based on FIFO worth of bytes to be
> - * sent at current baud. Add a little fluff to the wait.
> - */
> - timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
> - }
> + /*
> + * This function is used to poll bits, some of which (like CMD_DONE)
> + * might take as long as it takes for the FIFO plus the temp register
> + * on the geni side to drain. The Linux core calculates such a timeout
> + * for us and we can get it from uart_fifo_timeout().
> + *
> + * It should be noted that during earlycon the variables that
> + * uart_fifo_timeout() makes use of in "uport" may not be setup yet.
> + * It's difficult to set things up for earlycon since it can't
> + * necessarily figure out the baud rate and reading the FIFO depth
> + * from the wrapper means some extra MMIO maps that we don't get by
> + * default. This isn't a big problem, though, since uart_fifo_timeout()
> + * gives back its "slop" of 20ms as a minimum and that should be
> + * plenty of time for earlycon unless we're running at an extremely
> + * low baud rate.
> + */
> + timeout_us = jiffies_to_usecs(uart_fifo_timeout(uport));
Hi,
While this is not exactly incorrect, the back and forth conversions nsecs
-> jiffies -> usecs feels somewhat odd, perhaps reworking
uart_fifo_timeout()'s return type from jiffies to e.g. usecs would be
preferrable. As is, the jiffies as its return type seems a small obstacle
for using uart_fifo_timeout() which has come up in other contexts too.
> @@ -1224,7 +1224,6 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
> qcom_geni_serial_stop_rx(uport);
> /* baud rate */
> baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
> - port->baud = baud;
It's always nice to see this kind of cache variable removed, good work. :-)
--
i.
