Hi,
On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@xxxxxxxxxx> wrote:
>
> The qcom_geni_serial_poll_bit() can be used to wait for events like
> command completion and is supposed to wait for the time it takes to
> clear a full fifo before timing out.
>
> As noted by Doug, the current implementation does not account for start,
> stop and parity bits when determining the timeout. The helper also does
> not currently account for the shift register and the two-word
> intermediate transfer register.
>
> Instead of determining the fifo timeout on every call, store the timeout
> when updating it in set_termios() and wait for up to 19/16 the time it
> takes to clear the 16 word fifo to account for the shift and
> intermediate registers. Note that serial core has already added a 20 ms
> margin to the fifo timeout.
>
> Also note that the current uart_fifo_timeout() interface does
> unnecessary calculations on every call and also did not exists in
> earlier kernels so only store its result once. This also facilitates
> backports as earlier kernels can derive the timeout from uport->timeout,
> which has since been removed.
>
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Cc: stable@xxxxxxxxxxxxxxx # 4.17
> Reported-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 69a632fefc41..e1926124339d 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -124,7 +124,7 @@ struct qcom_geni_serial_port {
> dma_addr_t tx_dma_addr;
> dma_addr_t rx_dma_addr;
> bool setup;
> - unsigned int baud;
> + unsigned long fifo_timeout_us;
> unsigned long clk_rate;
> void *rx_buf;
> u32 loopback;
> @@ -270,22 +270,21 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> {
> 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;
>
> 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.
> + * Wait up to 19/16 the time it would take to clear a full
> + * FIFO, which accounts for the three words in the shift and
> + * intermediate registers.
> + *
> + * Note that fifo_timeout_us already has a 20 ms margin.
> */
> - timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
> + if (port->fifo_timeout_us)
> + timeout_us = 19 * port->fifo_timeout_us / 16;
It made me giggle a bit that part of the justification for caching
"fifo_timeout_us" was to avoid calculations each time through the
function. ...but then the code does the "19/16" math here instead of
just including it in the cache. ;-) ;-) ;-)
That being said, I'm not really a fan of the "19 / 16" anyway. The 16
value is calculated elsewhere in the code as:
port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se);
port->tx_fifo_width = geni_se_get_tx_fifo_width(&port->se);
port->rx_fifo_depth = geni_se_get_rx_fifo_depth(&port->se);
uport->fifosize =
(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
...and here you're just hardcoding it to 16. Then there's also the
fact that the "19 / 16" will also multiply the 20 ms "slop" added by
uart_fifo_timeout() which doesn't seem ideal.
How about this: we just change "uport->fifosize" to account for the 3
extra words? So it can be:
((port->tx_fifo_depth + 3) * port->tx_fifo_width) / BITS_PER_BYTE;
...then the cache will be correct and everything will work out. What
do you think?
-Doug
