| Message ID | 20240604090028.v3.2.I3e1968bbeee67e28fd4e15509950805b6665484a@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: qcom-geni: Overhaul TX handling to fix crashes/hangs | expand |
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@linux.intel.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > 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. :-)
Hi, On Fri, Jun 7, 2024 at 12:50 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > > + /* > > + * 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. Sure. I'll change it to "ms" instead of "us". We don't need the fidelity of "us" here given that the function is adding 20 ms of slop anyway so might as well return ms so that callers don't need to do so much math and don't need to work with u64. This means that I'll have to add a "* USEC_PER_MSEC" in my driver, but it still feels like the more correct thing to do. It also has the nice side effect of allowing the driver to remove the awkward "DIV_ROUND_UP(timeout_us, 10) * 10" because we know that the timeout will always be a proper multiple. I'll also add a new function with the _ms suffix instead of changing the old one. The suffix makes it clear to the caller what the unit of the returned value is and we might as well leave the old wrapper there--otherwise we just need to move the jiffies conversion into the existing callers.
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)); /* * Use custom implementation instead of readl_poll_atomic since ktimer @@ -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; sampling_rate = UART_OVERSAMPLING; /* Sampling rate is halved for IP versions >= 2.5 */
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@linux.intel.com> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- 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(-)