[PATCH 0/8] serial: qcom-geni: fix console corruption

[PATCH 0/8] serial: qcom-geni: fix console corruption

[Johan Hovold]

This series is a follow-on series to the lockup fixes [1] that addresses
a number of issues in the Qualcomm GENI console code, including corrupt
console output during boot, which is a problem for automated CI testing.

Johan

[1] https://lore.kernel.org/lkml/20240704101805.30612-1-johan+linaro@kernel.org/


Douglas Anderson (3):
  soc: qcom: geni-se: add GP_LENGTH/IRQ_EN_SET/IRQ_EN_CLEAR registers
  serial: qcom-geni: fix arg types for qcom_geni_serial_poll_bit()
  serial: qcom-geni: introduce qcom_geni_serial_poll_bitfield()

Johan Hovold (5):
  serial: qcom-geni: fix fifo polling timeout
  serial: qcom-geni: fix false console tx restart
  serial: qcom-geni: fix console corruption
  serial: qcom-geni: disable interrupts during console writes
  serial: qcom-geni: fix polled console corruption

 drivers/tty/serial/qcom_geni_serial.c | 133 +++++++++++++++-----------
 include/linux/soc/qcom/geni-se.h      |   9 ++
 2 files changed, 85 insertions(+), 57 deletions(-)

-- 
2.44.2

[PATCH 1/8] serial: qcom-geni: fix fifo polling timeout

[Johan Hovold]

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@vger.kernel.org	# 4.17
Reported-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 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;
 	}
 
 	/*
@@ -1248,7 +1247,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 */
@@ -1326,8 +1324,10 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	else
 		tx_trans_cfg |= UART_CTS_MASK;
 
-	if (baud)
+	if (baud) {
 		uart_update_timeout(uport, termios->c_cflag, baud);
+		port->fifo_timeout_us = jiffies_to_usecs(uart_fifo_timeout(uport));
+	}
 
 	if (!uart_console(uport))
 		writel(port->loopback,
-- 
2.44.2

[PATCH 2/8] serial: qcom-geni: fix false console tx restart

[Johan Hovold]

Commit 663abb1a7a7f ("tty: serial: qcom_geni_serial: Fix UART hang")
addressed an issue with stalled tx after the console code interrupted
the last bytes of a tx command by reenabling the watermark interrupt if
there is data in write buffer. This can however break software flow
control by re-enabling tx after the user has stopped it.

Address the original issue by not clearing the CMD_DONE flag after
polling for command completion. This allows the interrupt handler to
start another transfer when the CMD_DONE interrupt has not been disabled
due to flow control.

Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
Fixes: 663abb1a7a7f ("tty: serial: qcom_geni_serial: Fix UART hang")
Cc: stable@vger.kernel.org	# 4.17
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index e1926124339d..0af4a93c0af5 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -314,18 +314,16 @@ static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
 static void qcom_geni_serial_poll_tx_done(struct uart_port *uport)
 {
 	int done;
-	u32 irq_clear = M_CMD_DONE_EN;
 
 	done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 						M_CMD_DONE_EN, true);
 	if (!done) {
 		writel(M_GENI_CMD_ABORT, uport->membase +
 						SE_GENI_M_CMD_CTRL_REG);
-		irq_clear |= M_CMD_ABORT_EN;
 		qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 							M_CMD_ABORT_EN, true);
+		writel(M_CMD_ABORT_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 	}
-	writel(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
 }
 
 static void qcom_geni_serial_abort_rx(struct uart_port *uport)
@@ -386,6 +384,7 @@ static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
 							unsigned char c)
 {
 	writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
+	writel(M_CMD_DONE_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 	qcom_geni_serial_setup_tx(uport, 1);
 	WARN_ON(!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 						M_TX_FIFO_WATERMARK_EN, true));
@@ -430,6 +429,7 @@ __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
 	}
 
 	writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
+	writel(M_CMD_DONE_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 	qcom_geni_serial_setup_tx(uport, bytes_to_send);
 	for (i = 0; i < count; ) {
 		size_t chars_to_write = 0;
@@ -471,7 +471,6 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 	bool locked = true;
 	unsigned long flags;
 	u32 geni_status;
-	u32 irq_en;
 
 	WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
 
@@ -503,12 +502,6 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 		 * has been sent, in which case we need to look for done first.
 		 */
 		qcom_geni_serial_poll_tx_done(uport);
-
-		if (!kfifo_is_empty(&uport->state->port.xmit_fifo)) {
-			irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
-			writel(irq_en | M_TX_FIFO_WATERMARK_EN,
-					uport->membase + SE_GENI_M_IRQ_EN);
-		}
 	}
 
 	__qcom_geni_serial_console_write(uport, s, count);
-- 
2.44.2

[PATCH 3/8] soc: qcom: geni-se: add GP_LENGTH/IRQ_EN_SET/IRQ_EN_CLEAR registers

[Johan Hovold]

From: Douglas Anderson <dianders@chromium.org>

For UART devices the M_GP_LENGTH is the TX word count. For other
devices this is the transaction word count.

For UART devices the S_GP_LENGTH is the RX word count.

The IRQ_EN set/clear registers allow you to set or clear bits in the
IRQ_EN register without needing a read-modify-write.

Acked-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Link: https://lore.kernel.org/r/20240610152420.v4.1.Ife7ced506aef1be3158712aa3ff34a006b973559@changeid
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 include/linux/soc/qcom/geni-se.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
index 0f038a1a0330..c3bca9c0bf2c 100644
--- a/include/linux/soc/qcom/geni-se.h
+++ b/include/linux/soc/qcom/geni-se.h
@@ -88,11 +88,15 @@ struct geni_se {
 #define SE_GENI_M_IRQ_STATUS		0x610
 #define SE_GENI_M_IRQ_EN		0x614
 #define SE_GENI_M_IRQ_CLEAR		0x618
+#define SE_GENI_M_IRQ_EN_SET		0x61c
+#define SE_GENI_M_IRQ_EN_CLEAR		0x620
 #define SE_GENI_S_CMD0			0x630
 #define SE_GENI_S_CMD_CTRL_REG		0x634
 #define SE_GENI_S_IRQ_STATUS		0x640
 #define SE_GENI_S_IRQ_EN		0x644
 #define SE_GENI_S_IRQ_CLEAR		0x648
+#define SE_GENI_S_IRQ_EN_SET		0x64c
+#define SE_GENI_S_IRQ_EN_CLEAR		0x650
 #define SE_GENI_TX_FIFOn		0x700
 #define SE_GENI_RX_FIFOn		0x780
 #define SE_GENI_TX_FIFO_STATUS		0x800
@@ -101,6 +105,8 @@ struct geni_se {
 #define SE_GENI_RX_WATERMARK_REG	0x810
 #define SE_GENI_RX_RFR_WATERMARK_REG	0x814
 #define SE_GENI_IOS			0x908
+#define SE_GENI_M_GP_LENGTH		0x910
+#define SE_GENI_S_GP_LENGTH		0x914
 #define SE_DMA_TX_IRQ_STAT		0xc40
 #define SE_DMA_TX_IRQ_CLR		0xc44
 #define SE_DMA_TX_FSM_RST		0xc58
@@ -234,6 +240,9 @@ struct geni_se {
 #define IO2_DATA_IN			BIT(1)
 #define RX_DATA_IN			BIT(0)
 
+/* SE_GENI_M_GP_LENGTH and SE_GENI_S_GP_LENGTH fields */
+#define GP_LENGTH			GENMASK(31, 0)
+
 /* SE_DMA_TX_IRQ_STAT Register fields */
 #define TX_DMA_DONE			BIT(0)
 #define TX_EOT				BIT(1)
-- 
2.44.2

[PATCH 4/8] serial: qcom-geni: fix arg types for qcom_geni_serial_poll_bit()

[Johan Hovold]

From: Douglas Anderson <dianders@chromium.org>

The "offset" passed in should be unsigned since it's always a positive
offset from our memory mapped IO.

The "field" should be u32 since we're anding it with a 32-bit value
read from the device.

Suggested-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Link: https://lore.kernel.org/r/20240610152420.v4.4.I24a0de52dd7336908df180fa6b698e001f3aff82@changeid
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 0af4a93c0af5..4625a2e5ebfb 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -266,7 +266,7 @@ static bool qcom_geni_serial_secondary_active(struct uart_port *uport)
 }
 
 static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
-				int offset, int field, bool set)
+				      unsigned int offset, u32 field, bool set)
 {
 	u32 reg;
 	struct qcom_geni_serial_port *port;
-- 
2.44.2

[PATCH 5/8] serial: qcom-geni: introduce qcom_geni_serial_poll_bitfield()

[Johan Hovold]

From: Douglas Anderson <dianders@chromium.org>

With a small modification the qcom_geni_serial_poll_bit() function
could be used to poll more than just a single bit. Let's generalize
it. We'll make the qcom_geni_serial_poll_bit() into just a wrapper of
the general function.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Link: https://lore.kernel.org/r/20240610152420.v4.5.Ic6411eab8d9d37acc451705f583fb535cd6dadb2@changeid
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 4625a2e5ebfb..7029c39a9a21 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -265,8 +265,8 @@ static bool qcom_geni_serial_secondary_active(struct uart_port *uport)
 	return readl(uport->membase + SE_GENI_STATUS) & S_GENI_CMD_ACTIVE;
 }
 
-static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
-				      unsigned int offset, u32 field, bool set)
+static bool qcom_geni_serial_poll_bitfield(struct uart_port *uport,
+					   unsigned int offset, u32 field, u32 val)
 {
 	u32 reg;
 	struct qcom_geni_serial_port *port;
@@ -294,7 +294,7 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
 	timeout_us = DIV_ROUND_UP(timeout_us, 10) * 10;
 	while (timeout_us) {
 		reg = readl(uport->membase + offset);
-		if ((bool)(reg & field) == set)
+		if ((reg & field) == val)
 			return true;
 		udelay(10);
 		timeout_us -= 10;
@@ -302,6 +302,12 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
 	return false;
 }
 
+static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
+				      unsigned int offset, u32 field, bool set)
+{
+	return qcom_geni_serial_poll_bitfield(uport, offset, field, set ? field : 0);
+}
+
 static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
 {
 	u32 m_cmd;
-- 
2.44.2

[PATCH 6/8] serial: qcom-geni: fix console corruption

[Johan Hovold]

The Qualcomm serial console implementation is broken and can lose
characters when the serial port is also used for tty output.

Specifically, the console code only waits for the current tx command to
complete when all data has already been written to the fifo. When there
are on-going longer transfers this often means that console output is
lost when the console code inadvertently "hijacks" the current tx
command instead of starting a new one.

This can, for example, be observed during boot when console output that
should have been interspersed with init output is truncated:

	[    9.462317] qcom-snps-eusb2-hsphy fde000.phy: Registered Qcom-eUSB2 phy
	[  OK  ] Found device KBG50ZNS256G KIOXIA Wi[    9.471743ndows.
	[    9.539915] xhci-hcd xhci-hcd.0.auto: xHCI Host Controller

Add a new state variable to track how much data has been written to the
fifo and use it to determine when the fifo and shift register are both
empty. This is needed since there is currently no other known way to
determine when the shift register is empty.

This in turn allows the console code to interrupt long transfers without
losing data.

Note that the oops-in-progress case is similarly broken as it does not
cancel any active command and also waits for the wrong status flag when
attempting to drain the fifo (TX_FIFO_NOT_EMPTY_EN is only set when
cancelling a command leaves data in the fifo).

Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
Fixes: a1fee899e5be ("tty: serial: qcom_geni_serial: Fix softlock")
Fixes: 9e957a155005 ("serial: qcom-geni: Don't cancel/abort if we can't get the port lock")
Cc: stable@vger.kernel.org	# 4.17
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 48 ++++++++++++++-------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 7029c39a9a21..be620c5703f5 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -131,6 +131,7 @@ struct qcom_geni_serial_port {
 	bool brk;
 
 	unsigned int tx_remaining;
+	unsigned int tx_queued;
 	int wakeup_irq;
 	bool rx_tx_swap;
 	bool cts_rts_swap;
@@ -144,6 +145,8 @@ static const struct uart_ops qcom_geni_uart_pops;
 static struct uart_driver qcom_geni_console_driver;
 static struct uart_driver qcom_geni_uart_driver;
 
+static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport);
+
 static inline struct qcom_geni_serial_port *to_dev_port(struct uart_port *uport)
 {
 	return container_of(uport, struct qcom_geni_serial_port, uport);
@@ -308,6 +311,17 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
 	return qcom_geni_serial_poll_bitfield(uport, offset, field, set ? field : 0);
 }
 
+static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+
+	if (!qcom_geni_serial_main_active(uport))
+		return;
+
+	qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, GP_LENGTH,
+			port->tx_queued);
+}
+
 static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
 {
 	u32 m_cmd;
@@ -476,7 +490,6 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 	struct qcom_geni_serial_port *port;
 	bool locked = true;
 	unsigned long flags;
-	u32 geni_status;
 
 	WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
 
@@ -490,34 +503,20 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 	else
 		uart_port_lock_irqsave(uport, &flags);
 
-	geni_status = readl(uport->membase + SE_GENI_STATUS);
+	if (qcom_geni_serial_main_active(uport)) {
+		/* Wait for completion or drain FIFO */
+		if (!locked || port->tx_remaining == 0)
+			qcom_geni_serial_poll_tx_done(uport);
+		else
+			qcom_geni_serial_drain_fifo(uport);
 
-	if (!locked) {
-		/*
-		 * We can only get here if an oops is in progress then we were
-		 * unable to get the lock. This means we can't safely access
-		 * our state variables like tx_remaining. About the best we
-		 * can do is wait for the FIFO to be empty before we start our
-		 * transfer, so we'll do that.
-		 */
-		qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
-					  M_TX_FIFO_NOT_EMPTY_EN, false);
-	} else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
-		/*
-		 * It seems we can't interrupt existing transfers if all data
-		 * has been sent, in which case we need to look for done first.
-		 */
-		qcom_geni_serial_poll_tx_done(uport);
+		qcom_geni_serial_cancel_tx_cmd(uport);
 	}
 
 	__qcom_geni_serial_console_write(uport, s, count);
 
-
-	if (locked) {
-		if (port->tx_remaining)
-			qcom_geni_serial_setup_tx(uport, port->tx_remaining);
+	if (locked)
 		uart_port_unlock_irqrestore(uport, flags);
-	}
 }
 
 static void handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
@@ -698,6 +697,7 @@ static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport)
 	writel(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 
 	port->tx_remaining = 0;
+	port->tx_queued = 0;
 }
 
 static void qcom_geni_serial_handle_rx_fifo(struct uart_port *uport, bool drop)
@@ -924,6 +924,7 @@ static void qcom_geni_serial_handle_tx_fifo(struct uart_port *uport,
 	if (!port->tx_remaining) {
 		qcom_geni_serial_setup_tx(uport, pending);
 		port->tx_remaining = pending;
+		port->tx_queued = 0;
 
 		irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
 		if (!(irq_en & M_TX_FIFO_WATERMARK_EN))
@@ -932,6 +933,7 @@ static void qcom_geni_serial_handle_tx_fifo(struct uart_port *uport,
 	}
 
 	qcom_geni_serial_send_chunk_fifo(uport, chunk);
+	port->tx_queued += chunk;
 
 	/*
 	 * The tx fifo watermark is level triggered and latched. Though we had
-- 
2.44.2

[PATCH 7/8] serial: qcom-geni: disable interrupts during console writes

[Johan Hovold]

Disable the GENI interrupts during console writes to reduce the risk of
having interrupt handlers spinning on the port lock on other cores for
extended periods of time.

This can, for example, reduce the total amount of time spent in the
interrupt handler during boot of the x1e80100 CRD by up to a factor nine
(e.g. from 274 ms to 30 ms) while the worst case processing time drops
from 19 ms to 8 ms.

Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index be620c5703f5..fbed143c90a3 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -488,6 +488,7 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 {
 	struct uart_port *uport;
 	struct qcom_geni_serial_port *port;
+	u32 m_irq_en, s_irq_en;
 	bool locked = true;
 	unsigned long flags;
 
@@ -503,6 +504,11 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 	else
 		uart_port_lock_irqsave(uport, &flags);
 
+	m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+	s_irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
+	writel(0, uport->membase + SE_GENI_M_IRQ_EN);
+	writel(0, uport->membase + SE_GENI_S_IRQ_EN);
+
 	if (qcom_geni_serial_main_active(uport)) {
 		/* Wait for completion or drain FIFO */
 		if (!locked || port->tx_remaining == 0)
@@ -515,6 +521,9 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 
 	__qcom_geni_serial_console_write(uport, s, count);
 
+	writel(m_irq_en, uport->membase + SE_GENI_M_IRQ_EN);
+	writel(s_irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+
 	if (locked)
 		uart_port_unlock_irqrestore(uport, flags);
 }
-- 
2.44.2

[PATCH 8/8] serial: qcom-geni: fix polled console corruption

[Johan Hovold]

The polled UART operations are used by the kernel debugger (KDB, KGDB),
which can interrupt the kernel at any point in time. The current
Qualcomm GENI implementation does not really work when there is on-going
serial output as it inadvertently "hijacks" the current tx command,
which can result in both the initial debugger output being corrupted as
well as the corruption of any on-going serial output (up to 4k
characters) when execution resumes:

0190: abcdefghijklmnopqrstuvwxyz0123456789 0190: abcdefghijklmnopqrstuvwxyz0123456789
0191: abcdefghijklmnop[   50.825552] sysrq: DEBUG
qrstuvwxyz0123456789 0191: abcdefghijklmnopqrstuvwxyz0123456789
Entering kdb (current=0xffff53510b4cd280, pid 640) on processor 2 due to Keyboard Entry
[2]kdb> go
omlji3h3h2g2g1f1f0e0ezdzdycycxbxbwawav :t72r2rp
o9n976k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawavu:t7t8s8s8r2r2q0q0p
o9n9n8ml6k6k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawav v u:u:t9t0s4s4rq0p
o9n9n8m8m7l7l6k6k5j5j40q0p                                              p o
o9n9n8m8m7l7l6k6k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawav :t8t9s4s4r4r4q0q0p

Fix this by making sure that the polled output implementation waits for
the tx fifo to drain before cancelling any on-going longer transfers. As
the polled code cannot take any locks, leave the state variables as they
are and instead make sure that the interrupt handler always starts a new
tx command when there is data in the write buffer.

Since the debugger can interrupt the interrupt handler when it is
writing data to the tx fifo, it is currently not possible to fully
prevent losing up to 64 bytes of tty output on resume.

Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
Cc: stable@vger.kernel.org      # 4.17
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index fbed143c90a3..cf8bafd99a09 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -145,6 +145,7 @@ static const struct uart_ops qcom_geni_uart_pops;
 static struct uart_driver qcom_geni_console_driver;
 static struct uart_driver qcom_geni_uart_driver;
 
+static void __qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport);
 static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport);
 
 static inline struct qcom_geni_serial_port *to_dev_port(struct uart_port *uport)
@@ -403,13 +404,14 @@ static int qcom_geni_serial_get_char(struct uart_port *uport)
 static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
 							unsigned char c)
 {
-	writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
+	if (qcom_geni_serial_main_active(uport)) {
+		qcom_geni_serial_poll_tx_done(uport);
+		__qcom_geni_serial_cancel_tx_cmd(uport);
+	}
+
 	writel(M_CMD_DONE_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 	qcom_geni_serial_setup_tx(uport, 1);
-	WARN_ON(!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
-						M_TX_FIFO_WATERMARK_EN, true));
 	writel(c, uport->membase + SE_GENI_TX_FIFOn);
-	writel(M_TX_FIFO_WATERMARK_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 	qcom_geni_serial_poll_tx_done(uport);
 }
 #endif
@@ -688,13 +690,10 @@ static void qcom_geni_serial_stop_tx_fifo(struct uart_port *uport)
 	writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
 }
 
-static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport)
+static void __qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport)
 {
 	struct qcom_geni_serial_port *port = to_dev_port(uport);
 
-	if (!qcom_geni_serial_main_active(uport))
-		return;
-
 	geni_se_cancel_m_cmd(&port->se);
 	if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 						M_CMD_CANCEL_EN, true)) {
@@ -704,6 +703,16 @@ static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport)
 		writel(M_CMD_ABORT_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
 	}
 	writel(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
+}
+
+static void qcom_geni_serial_cancel_tx_cmd(struct uart_port *uport)
+{
+	struct qcom_geni_serial_port *port = to_dev_port(uport);
+
+	if (!qcom_geni_serial_main_active(uport))
+		return;
+
+	__qcom_geni_serial_cancel_tx_cmd(uport);
 
 	port->tx_remaining = 0;
 	port->tx_queued = 0;
@@ -930,7 +939,7 @@ static void qcom_geni_serial_handle_tx_fifo(struct uart_port *uport,
 	if (!chunk)
 		goto out_write_wakeup;
 
-	if (!port->tx_remaining) {
+	if (!active) {
 		qcom_geni_serial_setup_tx(uport, pending);
 		port->tx_remaining = pending;
 		port->tx_queued = 0;
-- 
2.44.2

Re: [PATCH 6/8] serial: qcom-geni: fix console corruption

[kernel test robot]

Hi Johan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.11-rc6 next-20240902]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Johan-Hovold/serial-qcom-geni-fix-fifo-polling-timeout/20240902-232801
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20240902152451.862-7-johan%2Blinaro%40kernel.org
patch subject: [PATCH 6/8] serial: qcom-geni: fix console corruption
config: arc-randconfig-001-20240903 (https://download.01.org/0day-ci/archive/20240903/202409031521.hMYVfAjO-lkp@intel.com/config)
compiler: arc-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240903/202409031521.hMYVfAjO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409031521.hMYVfAjO-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/tty/serial/qcom_geni_serial.c:314:13: warning: 'qcom_geni_serial_drain_fifo' defined but not used [-Wunused-function]
     314 | static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/qcom_geni_serial_drain_fifo +314 drivers/tty/serial/qcom_geni_serial.c

   313	
 > 314	static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
   315	{
   316		struct qcom_geni_serial_port *port = to_dev_port(uport);
   317	
   318		if (!qcom_geni_serial_main_active(uport))
   319			return;
   320	
   321		qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, GP_LENGTH,
   322				port->tx_queued);
   323	}
   324	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/8] serial: qcom-geni: fix console corruption

[Nícolas F. R. A. Prado]

On Mon, Sep 02, 2024 at 05:24:43PM +0200, Johan Hovold wrote:
> This series is a follow-on series to the lockup fixes [1] that addresses
> a number of issues in the Qualcomm GENI console code, including corrupt
> console output during boot, which is a problem for automated CI testing.
> 
> Johan
> 
> [1] https://lore.kernel.org/lkml/20240704101805.30612-1-johan+linaro@kernel.org/
> 
> 
> Douglas Anderson (3):
>   soc: qcom: geni-se: add GP_LENGTH/IRQ_EN_SET/IRQ_EN_CLEAR registers
>   serial: qcom-geni: fix arg types for qcom_geni_serial_poll_bit()
>   serial: qcom-geni: introduce qcom_geni_serial_poll_bitfield()
> 
> Johan Hovold (5):
>   serial: qcom-geni: fix fifo polling timeout
>   serial: qcom-geni: fix false console tx restart
>   serial: qcom-geni: fix console corruption
>   serial: qcom-geni: disable interrupts during console writes
>   serial: qcom-geni: fix polled console corruption
> 
>  drivers/tty/serial/qcom_geni_serial.c | 133 +++++++++++++++-----------
>  include/linux/soc/qcom/geni-se.h      |   9 ++
>  2 files changed, 85 insertions(+), 57 deletions(-)
> 
> -- 
> 2.44.2
> 

This series fixes the serial issues we're seeing on the sc7180 based
(sc7180-trogdor-lazor-limozeen and sc7180-trogdor-kingoftown) boards that we
have hooked up to KernelCI. Out of a 10-job batch of boot tests all succeeded
after the patch, whereas before most failed (7/10), due to a missing message in
the serial.

Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Looking forward to see this landed!

Thanks,
Nícolas

Re: [PATCH 1/8] serial: qcom-geni: fix fifo polling timeout

[Doug Anderson]

Hi,

On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> 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@vger.kernel.org      # 4.17
> Reported-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  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

Re: [PATCH 2/8] serial: qcom-geni: fix false console tx restart

[Doug Anderson]

Hi,

On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Commit 663abb1a7a7f ("tty: serial: qcom_geni_serial: Fix UART hang")
> addressed an issue with stalled tx after the console code interrupted
> the last bytes of a tx command by reenabling the watermark interrupt if
> there is data in write buffer. This can however break software flow
> control by re-enabling tx after the user has stopped it.
>
> Address the original issue by not clearing the CMD_DONE flag after
> polling for command completion. This allows the interrupt handler to
> start another transfer when the CMD_DONE interrupt has not been disabled
> due to flow control.
>
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Fixes: 663abb1a7a7f ("tty: serial: qcom_geni_serial: Fix UART hang")
> Cc: stable@vger.kernel.org      # 4.17
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)

This seems reasonable. I guess this can end up causing a spurious
"done" interrupt to sometimes occur but that looks to be harmless.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 6/8] serial: qcom-geni: fix console corruption

[Doug Anderson]

Hi,

On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> +static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
> +{
> +       struct qcom_geni_serial_port *port = to_dev_port(uport);
> +
> +       if (!qcom_geni_serial_main_active(uport))
> +               return;

It seems like all callers already do the check and only ever call you
if the port is active. Do you really need to re-check?


> @@ -308,6 +311,17 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
>         return qcom_geni_serial_poll_bitfield(uport, offset, field, set ? field : 0);
>  }
>
> +static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
> +{
> +       struct qcom_geni_serial_port *port = to_dev_port(uport);
> +
> +       if (!qcom_geni_serial_main_active(uport))
> +               return;
> +
> +       qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, GP_LENGTH,
> +                       port->tx_queued);

nit: indent "port->tx_queued" to match open parenthesis?

...also: as the kernel test robot reported, w/ certain CONFIGs this is
defined / not used.

Aside from the nit / robot issue, this solution looks reasonable to
me. It's been long enough that I've already paged out much of the past
digging I did into this driver, but this seems like it should work.
Feel free to add my Reviewed-by when the robot issue is fixed.



-Doug

Re: [PATCH 7/8] serial: qcom-geni: disable interrupts during console writes

[Doug Anderson]

Hi,

On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> Disable the GENI interrupts during console writes to reduce the risk of
> having interrupt handlers spinning on the port lock on other cores for
> extended periods of time.
>
> This can, for example, reduce the total amount of time spent in the
> interrupt handler during boot of the x1e80100 CRD by up to a factor nine
> (e.g. from 274 ms to 30 ms) while the worst case processing time drops
> from 19 ms to 8 ms.
>
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 8/8] serial: qcom-geni: fix polled console corruption

[Doug Anderson]

Hi,

On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The polled UART operations are used by the kernel debugger (KDB, KGDB),
> which can interrupt the kernel at any point in time. The current
> Qualcomm GENI implementation does not really work when there is on-going
> serial output as it inadvertently "hijacks" the current tx command,
> which can result in both the initial debugger output being corrupted as
> well as the corruption of any on-going serial output (up to 4k
> characters) when execution resumes:
>
> 0190: abcdefghijklmnopqrstuvwxyz0123456789 0190: abcdefghijklmnopqrstuvwxyz0123456789
> 0191: abcdefghijklmnop[   50.825552] sysrq: DEBUG
> qrstuvwxyz0123456789 0191: abcdefghijklmnopqrstuvwxyz0123456789
> Entering kdb (current=0xffff53510b4cd280, pid 640) on processor 2 due to Keyboard Entry
> [2]kdb> go
> omlji3h3h2g2g1f1f0e0ezdzdycycxbxbwawav :t72r2rp
> o9n976k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawavu:t7t8s8s8r2r2q0q0p
> o9n9n8ml6k6k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawav v u:u:t9t0s4s4rq0p
> o9n9n8m8m7l7l6k6k5j5j40q0p                                              p o
> o9n9n8m8m7l7l6k6k5j5j4i4i3h3h2g2g1f1f0e0ezdzdycycxbxbwawav :t8t9s4s4r4r4q0q0p
>
> Fix this by making sure that the polled output implementation waits for
> the tx fifo to drain before cancelling any on-going longer transfers. As
> the polled code cannot take any locks, leave the state variables as they
> are and instead make sure that the interrupt handler always starts a new
> tx command when there is data in the write buffer.
>
> Since the debugger can interrupt the interrupt handler when it is
> writing data to the tx fifo, it is currently not possible to fully
> prevent losing up to 64 bytes of tty output on resume.
>
> Fixes: c4f528795d1a ("tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP")
> Cc: stable@vger.kernel.org      # 4.17
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 0/8] serial: qcom-geni: fix console corruption

[Johan Hovold]

On Wed, Sep 04, 2024 at 02:08:54PM -0400, Nícolas F. R. A. Prado wrote:
> On Mon, Sep 02, 2024 at 05:24:43PM +0200, Johan Hovold wrote:
> > This series is a follow-on series to the lockup fixes [1] that addresses
> > a number of issues in the Qualcomm GENI console code, including corrupt
> > console output during boot, which is a problem for automated CI testing.

> This series fixes the serial issues we're seeing on the sc7180 based
> (sc7180-trogdor-lazor-limozeen and sc7180-trogdor-kingoftown) boards that we
> have hooked up to KernelCI. Out of a 10-job batch of boot tests all succeeded
> after the patch, whereas before most failed (7/10), due to a missing message in
> the serial.
> 
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thanks for testing!

Johan

Re: [PATCH 1/8] serial: qcom-geni: fix fifo polling timeout

[Johan Hovold]

On Wed, Sep 04, 2024 at 02:50:57PM -0700, Doug Anderson wrote:
> On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> 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.

> > @@ -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. ;-) ;-) ;-)

Heh, yeah, but I was really talking about uart_fifo_timeout() doing
unnecessary calculations on each call (and that value used to be
calculated once and stored for later use).

I also realised that we need to account for the intermediate register
after I wrote the initial commit message, and before that this was just
a shift and add.

> 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.

Indeed, and the early console code also hardcodes this to 16.

I don't care about the slop being 20 ms or 23.5, this is just a timeout
for the error case.

This will over count a bit if there is uart hw with 256 B fifos, but
could potentially undercount if there is hw with less than 16 words. I'm
not sure if such hw exists, but I'll see what I can find out.

> 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?

I don't think uart_fifo_timeout traditionally accounts for the shift
register and we wait up to *twice* the time it takes to clear to fifo
anyway (in wait_until_sent). The intermediate register I found here
could perhaps be considered part of the fifo however.

I'll give this some more thought.

Johan

Re: [PATCH 6/8] serial: qcom-geni: fix console corruption

[Johan Hovold]

On Wed, Sep 04, 2024 at 02:51:15PM -0700, Doug Anderson wrote:
> On Mon, Sep 2, 2024 at 8:26 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > +static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
> > +{
> > +       struct qcom_geni_serial_port *port = to_dev_port(uport);
> > +
> > +       if (!qcom_geni_serial_main_active(uport))
> > +               return;
> 
> It seems like all callers already do the check and only ever call you
> if the port is active. Do you really need to re-check?

I wanted to make the helper self-contained and work in both cases. But
since I ended up only using this helper only in the console code and
will need to move it anyway (under the console ifdef), perhaps I can
consider dropping it. But then again, it's just one register read.

> > @@ -308,6 +311,17 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> >         return qcom_geni_serial_poll_bitfield(uport, offset, field, set ? field : 0);
> >  }
> >
> > +static void qcom_geni_serial_drain_fifo(struct uart_port *uport)
> > +{
> > +       struct qcom_geni_serial_port *port = to_dev_port(uport);
> > +
> > +       if (!qcom_geni_serial_main_active(uport))
> > +               return;
> > +
> > +       qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, GP_LENGTH,
> > +                       port->tx_queued);
> 
> nit: indent "port->tx_queued" to match open parenthesis?

No, I don't use open-parenthesis alignment unless that's the
(consistent) style of the code I'm changing (e.g. to avoid unnecessary
realignments when symbol names change and to make a point about
checkpatch --pedantic warnings not being part of the coding style).
 
> ...also: as the kernel test robot reported, w/ certain CONFIGs this is
> defined / not used.

Yes, I need to move the helper under the console ifdef. I was just
waiting to see if there was any further feedback before respinning.

> Aside from the nit / robot issue, this solution looks reasonable to
> me. It's been long enough that I've already paged out much of the past
> digging I did into this driver, but this seems like it should work.
> Feel free to add my Reviewed-by when the robot issue is fixed.

Thanks for reviewing.

Johan

Re: [PATCH 1/8] serial: qcom-geni: fix fifo polling timeout

[Johan Hovold]

On Thu, Sep 05, 2024 at 10:48:13AM +0200, Johan Hovold wrote:
> On Wed, Sep 04, 2024 at 02:50:57PM -0700, Doug Anderson wrote:

> > 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?
> 
> I don't think uart_fifo_timeout traditionally accounts for the shift
> register and we wait up to *twice* the time it takes to clear to fifo
> anyway (in wait_until_sent). The intermediate register I found here
> could perhaps be considered part of the fifo however.
> 
> I'll give this some more thought.

I decided to keep the fifo size as-is (e.g. as it is exported to user
space) and only account for the shift and intermediate registers in the
driver. I'm using the fifo size reported by the hardware to determine
the timeout in v2.

Johan