Re: [PATCH v3 2/7] serial: qcom-geni: Fix the timeout in qcom_geni_serial_poll_bit()

[Ilpo Järvinen]

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.