[PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups

[PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups

[Ryan Case]

This is a series of cleanups for issues raised in prior reviews that
were unrelated to the patches at hand.


Ryan Case (4):
  tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb()
  tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related
    variables
  tty: serial: qcom_geni_serial: Remove xfer_mode variable
  tty: serial: qcom_geni_serial: Use u32 for register variables

 drivers/tty/serial/qcom_geni_serial.c | 270 ++++++++++----------------
 1 file changed, 104 insertions(+), 166 deletions(-)

-- 
2.20.1.415.g653613c723-goog

[PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb()

[Ryan Case]

A frequent side comment has been to remove the use of writel_relaxed,
readl_relaxed, and mb.

Signed-off-by: Ryan Case <ryandcase@chromium.org>
---

 drivers/tty/serial/qcom_geni_serial.c | 187 +++++++++++---------------
 1 file changed, 80 insertions(+), 107 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 2b1e73193dad..dc95b96148ed 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -230,7 +230,7 @@ static unsigned int qcom_geni_serial_get_mctrl(struct uart_port *uport)
 	if (uart_console(uport) || !uart_cts_enabled(uport)) {
 		mctrl |= TIOCM_CTS;
 	} else {
-		geni_ios = readl_relaxed(uport->membase + SE_GENI_IOS);
+		geni_ios = readl(uport->membase + SE_GENI_IOS);
 		if (!(geni_ios & IO2_DATA_IN))
 			mctrl |= TIOCM_CTS;
 	}
@@ -248,7 +248,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,
 
 	if (!(mctrl & TIOCM_RTS))
 		uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
-	writel_relaxed(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
+	writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
 }
 
 static const char *qcom_geni_serial_get_type(struct uart_port *uport)
@@ -277,9 +277,6 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
 	unsigned int fifo_bits;
 	unsigned long timeout_us = 20000;
 
-	/* Ensure polling is not re-ordered before the prior writes/reads */
-	mb();
-
 	if (uport->private_data) {
 		port = to_dev_port(uport, uport);
 		baud = port->baud;
@@ -299,7 +296,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_relaxed(uport->membase + offset);
+		reg = readl(uport->membase + offset);
 		if ((bool)(reg & field) == set)
 			return true;
 		udelay(10);
@@ -312,7 +309,7 @@ static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
 {
 	u32 m_cmd;
 
-	writel_relaxed(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
+	writel(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
 	m_cmd = UART_START_TX << M_OPCODE_SHFT;
 	writel(m_cmd, uport->membase + SE_GENI_M_CMD0);
 }
@@ -325,13 +322,13 @@ static void qcom_geni_serial_poll_tx_done(struct uart_port *uport)
 	done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 						M_CMD_DONE_EN, true);
 	if (!done) {
-		writel_relaxed(M_GENI_CMD_ABORT, uport->membase +
+		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_relaxed(irq_clear, 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)
@@ -341,8 +338,8 @@ static void qcom_geni_serial_abort_rx(struct uart_port *uport)
 	writel(S_GENI_CMD_ABORT, uport->membase + SE_GENI_S_CMD_CTRL_REG);
 	qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
 					S_GENI_CMD_ABORT, false);
-	writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
-	writel_relaxed(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
+	writel(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
+	writel(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
 }
 
 #ifdef CONFIG_CONSOLE_POLL
@@ -351,19 +348,13 @@ static int qcom_geni_serial_get_char(struct uart_port *uport)
 	u32 rx_fifo;
 	u32 status;
 
-	status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
-	writel_relaxed(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
-
-	status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
-	writel_relaxed(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
+	status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
+	writel(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
 
-	/*
-	 * Ensure the writes to clear interrupts is not re-ordered after
-	 * reading the data.
-	 */
-	mb();
+	status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
+	writel(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
 
-	status = readl_relaxed(uport->membase + SE_GENI_RX_FIFO_STATUS);
+	status = readl(uport->membase + SE_GENI_RX_FIFO_STATUS);
 	if (!(status & RX_FIFO_WC_MSK))
 		return NO_POLL_CHAR;
 
@@ -376,12 +367,12 @@ static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
 {
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 
-	writel_relaxed(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
+	writel(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
 	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_relaxed(c, uport->membase + SE_GENI_TX_FIFOn);
-	writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
+	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);
 }
@@ -390,7 +381,7 @@ static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
 #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
 static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
 {
-	writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn);
+	writel(ch, uport->membase + SE_GENI_TX_FIFOn);
 }
 
 static void
@@ -409,7 +400,7 @@ __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
 			bytes_to_send++;
 	}
 
-	writel_relaxed(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
+	writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
 	qcom_geni_serial_setup_tx(uport, bytes_to_send);
 	for (i = 0; i < count; ) {
 		size_t chars_to_write = 0;
@@ -427,7 +418,7 @@ __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
 		chars_to_write = min_t(size_t, count - i, avail / 2);
 		uart_console_write(uport, s + i, chars_to_write,
 						qcom_geni_serial_wr_char);
-		writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
+		writel(M_TX_FIFO_WATERMARK_EN, uport->membase +
 							SE_GENI_M_IRQ_CLEAR);
 		i += chars_to_write;
 	}
@@ -456,7 +447,7 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 	else
 		spin_lock_irqsave(&uport->lock, flags);
 
-	geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+	geni_status = readl(uport->membase + SE_GENI_STATUS);
 
 	/* Cancel the current write to log the fault */
 	if (!locked) {
@@ -466,10 +457,10 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 			geni_se_abort_m_cmd(&port->se);
 			qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 							M_CMD_ABORT_EN, true);
-			writel_relaxed(M_CMD_ABORT_EN, uport->membase +
+			writel(M_CMD_ABORT_EN, uport->membase +
 							SE_GENI_M_IRQ_CLEAR);
 		}
-		writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
+		writel(M_CMD_CANCEL_EN, uport->membase +
 							SE_GENI_M_IRQ_CLEAR);
 	} else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
 		/*
@@ -479,9 +470,9 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
 		qcom_geni_serial_poll_tx_done(uport);
 
 		if (uart_circ_chars_pending(&uport->state->xmit)) {
-			irq_en = readl_relaxed(uport->membase +
+			irq_en = readl(uport->membase +
 					SE_GENI_M_IRQ_EN);
-			writel_relaxed(irq_en | M_TX_FIFO_WATERMARK_EN,
+			writel(irq_en | M_TX_FIFO_WATERMARK_EN,
 					uport->membase + SE_GENI_M_IRQ_EN);
 		}
 	}
@@ -585,12 +576,12 @@ static void qcom_geni_serial_start_tx(struct uart_port *uport)
 		if (!qcom_geni_serial_tx_empty(uport))
 			return;
 
-		irq_en = readl_relaxed(uport->membase +	SE_GENI_M_IRQ_EN);
+		irq_en = readl(uport->membase +	SE_GENI_M_IRQ_EN);
 		irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
 
-		writel_relaxed(port->tx_wm, uport->membase +
+		writel(port->tx_wm, uport->membase +
 						SE_GENI_TX_WATERMARK_REG);
-		writel_relaxed(irq_en, uport->membase +	SE_GENI_M_IRQ_EN);
+		writel(irq_en, uport->membase +	SE_GENI_M_IRQ_EN);
 	}
 }
 
@@ -600,35 +591,29 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
 	u32 status;
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 
-	irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+	irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
 	irq_en &= ~M_CMD_DONE_EN;
 	if (port->xfer_mode == GENI_SE_FIFO) {
 		irq_en &= ~M_TX_FIFO_WATERMARK_EN;
-		writel_relaxed(0, uport->membase +
+		writel(0, uport->membase +
 				     SE_GENI_TX_WATERMARK_REG);
 	}
-	writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
-	status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+	writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
+	status = readl(uport->membase + SE_GENI_STATUS);
 	/* Possible stop tx is called multiple times. */
 	if (!(status & M_GENI_CMD_ACTIVE))
 		return;
 
-	/*
-	 * Ensure cancel command write is not re-ordered before checking
-	 * the status of the Primary Sequencer.
-	 */
-	mb();
-
 	geni_se_cancel_m_cmd(&port->se);
 	if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 						M_CMD_CANCEL_EN, true)) {
 		geni_se_abort_m_cmd(&port->se);
 		qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 						M_CMD_ABORT_EN, true);
-		writel_relaxed(M_CMD_ABORT_EN, uport->membase +
+		writel(M_CMD_ABORT_EN, uport->membase +
 							SE_GENI_M_IRQ_CLEAR);
 	}
-	writel_relaxed(M_CMD_CANCEL_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_start_rx(struct uart_port *uport)
@@ -637,26 +622,20 @@ static void qcom_geni_serial_start_rx(struct uart_port *uport)
 	u32 status;
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 
-	status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+	status = readl(uport->membase + SE_GENI_STATUS);
 	if (status & S_GENI_CMD_ACTIVE)
 		qcom_geni_serial_stop_rx(uport);
 
-	/*
-	 * Ensure setup command write is not re-ordered before checking
-	 * the status of the Secondary Sequencer.
-	 */
-	mb();
-
 	geni_se_setup_s_cmd(&port->se, UART_START_READ, 0);
 
 	if (port->xfer_mode == GENI_SE_FIFO) {
-		irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN);
+		irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
 		irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
-		writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+		writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
 
-		irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+		irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
 		irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
-		writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
+		writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
 	}
 }
 
@@ -668,31 +647,25 @@ static void qcom_geni_serial_stop_rx(struct uart_port *uport)
 	u32 irq_clear = S_CMD_DONE_EN;
 
 	if (port->xfer_mode == GENI_SE_FIFO) {
-		irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN);
+		irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
 		irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
-		writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+		writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
 
-		irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+		irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
 		irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
-		writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
+		writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
 	}
 
-	status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+	status = readl(uport->membase + SE_GENI_STATUS);
 	/* Possible stop rx is called multiple times. */
 	if (!(status & S_GENI_CMD_ACTIVE))
 		return;
 
-	/*
-	 * Ensure cancel command write is not re-ordered before checking
-	 * the status of the Secondary Sequencer.
-	 */
-	mb();
-
 	geni_se_cancel_s_cmd(&port->se);
 	qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
 					S_GENI_CMD_CANCEL, false);
-	status = readl_relaxed(uport->membase + SE_GENI_STATUS);
-	writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
+	status = readl(uport->membase + SE_GENI_STATUS);
+	writel(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
 	if (status & S_GENI_CMD_ACTIVE)
 		qcom_geni_serial_abort_rx(uport);
 }
@@ -706,7 +679,7 @@ static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop)
 	u32 total_bytes;
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 
-	status = readl_relaxed(uport->membase +	SE_GENI_RX_FIFO_STATUS);
+	status = readl(uport->membase +	SE_GENI_RX_FIFO_STATUS);
 	word_cnt = status & RX_FIFO_WC_MSK;
 	last_word_partial = status & RX_LAST;
 	last_word_byte_cnt = (status & RX_LAST_BYTE_VALID_MSK) >>
@@ -736,7 +709,7 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
 	unsigned int chunk;
 	int tail;
 
-	status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
+	status = readl(uport->membase + SE_GENI_TX_FIFO_STATUS);
 
 	/* Complete the current tx command before taking newly added data */
 	if (active)
@@ -762,9 +735,9 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
 		qcom_geni_serial_setup_tx(uport, pending);
 		port->tx_remaining = pending;
 
-		irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+		irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
 		if (!(irq_en & M_TX_FIFO_WATERMARK_EN))
-			writel_relaxed(irq_en | M_TX_FIFO_WATERMARK_EN,
+			writel(irq_en | M_TX_FIFO_WATERMARK_EN,
 					uport->membase + SE_GENI_M_IRQ_EN);
 	}
 
@@ -797,14 +770,14 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
 	 * cleared it in qcom_geni_serial_isr it will have already reasserted
 	 * so we must clear it again here after our writes.
 	 */
-	writel_relaxed(M_TX_FIFO_WATERMARK_EN,
+	writel(M_TX_FIFO_WATERMARK_EN,
 			uport->membase + SE_GENI_M_IRQ_CLEAR);
 
 out_write_wakeup:
 	if (!port->tx_remaining) {
-		irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
+		irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
 		if (irq_en & M_TX_FIFO_WATERMARK_EN)
-			writel_relaxed(irq_en & ~M_TX_FIFO_WATERMARK_EN,
+			writel(irq_en & ~M_TX_FIFO_WATERMARK_EN,
 					uport->membase + SE_GENI_M_IRQ_EN);
 	}
 
@@ -828,12 +801,12 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 		return IRQ_NONE;
 
 	spin_lock_irqsave(&uport->lock, flags);
-	m_irq_status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
-	s_irq_status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
-	geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
-	m_irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
-	writel_relaxed(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
-	writel_relaxed(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
+	m_irq_status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
+	s_irq_status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
+	geni_status = readl(uport->membase + SE_GENI_STATUS);
+	m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+	writel(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
+	writel(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
 
 	if (WARN_ON(m_irq_status & M_ILLEGAL_CMD_EN))
 		goto out_unlock;
@@ -931,7 +904,7 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 	get_tx_fifo_size(port);
 
 	set_rfr_wm(port);
-	writel_relaxed(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
+	writel(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
 	/*
 	 * Make an unconditional cancel on the main sequencer to reset
 	 * it else we could end up in data loss scenarios.
@@ -1035,10 +1008,10 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 	ser_clk_cfg |= clk_div << CLK_DIV_SHFT;
 
 	/* parity */
-	tx_trans_cfg = readl_relaxed(uport->membase + SE_UART_TX_TRANS_CFG);
-	tx_parity_cfg = readl_relaxed(uport->membase + SE_UART_TX_PARITY_CFG);
-	rx_trans_cfg = readl_relaxed(uport->membase + SE_UART_RX_TRANS_CFG);
-	rx_parity_cfg = readl_relaxed(uport->membase + SE_UART_RX_PARITY_CFG);
+	tx_trans_cfg = readl(uport->membase + SE_UART_TX_TRANS_CFG);
+	tx_parity_cfg = readl(uport->membase + SE_UART_TX_PARITY_CFG);
+	rx_trans_cfg = readl(uport->membase + SE_UART_RX_TRANS_CFG);
+	rx_parity_cfg = readl(uport->membase + SE_UART_RX_PARITY_CFG);
 	if (termios->c_cflag & PARENB) {
 		tx_trans_cfg |= UART_TX_PAR_EN;
 		rx_trans_cfg |= UART_RX_PAR_EN;
@@ -1094,17 +1067,17 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 		uart_update_timeout(uport, termios->c_cflag, baud);
 
 	if (!uart_console(uport))
-		writel_relaxed(port->loopback,
+		writel(port->loopback,
 				uport->membase + SE_UART_LOOPBACK_CFG);
-	writel_relaxed(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG);
-	writel_relaxed(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG);
-	writel_relaxed(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG);
-	writel_relaxed(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG);
-	writel_relaxed(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
-	writel_relaxed(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
-	writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
-	writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
-	writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
+	writel(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG);
+	writel(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG);
+	writel(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG);
+	writel(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG);
+	writel(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
+	writel(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
+	writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
+	writel(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
+	writel(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
 out_restart_rx:
 	qcom_geni_serial_start_rx(uport);
 }
@@ -1195,13 +1168,13 @@ static int __init qcom_geni_serial_earlycon_setup(struct earlycon_device *dev,
 	geni_se_init(&se, DEF_FIFO_DEPTH_WORDS / 2, DEF_FIFO_DEPTH_WORDS - 2);
 	geni_se_select_mode(&se, GENI_SE_FIFO);
 
-	writel_relaxed(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG);
-	writel_relaxed(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG);
-	writel_relaxed(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG);
-	writel_relaxed(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG);
-	writel_relaxed(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
-	writel_relaxed(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
-	writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
+	writel(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG);
+	writel(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG);
+	writel(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG);
+	writel(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG);
+	writel(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
+	writel(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
+	writel(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
 
 	dev->con->write = qcom_geni_serial_earlycon_write;
 	dev->con->setup = NULL;
-- 
2.20.1.415.g653613c723-goog

[PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables

[Ryan Case]

The variables of tx_wm and rx_wm were set to the same define value in
all cases, never updated, and the define was sometimes used
interchangably. Remove the variables/function and use the fixed value.

Signed-off-by: Ryan Case <ryandcase@chromium.org>
---

 drivers/tty/serial/qcom_geni_serial.c | 23 +++--------------------
 1 file changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index dc95b96148ed..5521ed4a0708 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -105,9 +105,6 @@ struct qcom_geni_serial_port {
 	u32 tx_fifo_depth;
 	u32 tx_fifo_width;
 	u32 rx_fifo_depth;
-	u32 tx_wm;
-	u32 rx_wm;
-	u32 rx_rfr;
 	enum geni_se_xfer_mode xfer_mode;
 	bool setup;
 	int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
@@ -365,9 +362,7 @@ 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)
 {
-	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
-
-	writel(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
+	writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
 	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));
@@ -579,7 +574,7 @@ static void qcom_geni_serial_start_tx(struct uart_port *uport)
 		irq_en = readl(uport->membase +	SE_GENI_M_IRQ_EN);
 		irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
 
-		writel(port->tx_wm, uport->membase +
+		writel(DEF_TX_WM, uport->membase +
 						SE_GENI_TX_WATERMARK_REG);
 		writel(irq_en, uport->membase +	SE_GENI_M_IRQ_EN);
 	}
@@ -852,17 +847,6 @@ static void get_tx_fifo_size(struct qcom_geni_serial_port *port)
 		(port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
 }
 
-static void set_rfr_wm(struct qcom_geni_serial_port *port)
-{
-	/*
-	 * Set RFR (Flow off) to FIFO_DEPTH - 2.
-	 * RX WM level at 10% RX_FIFO_DEPTH.
-	 * TX WM level at 10% TX_FIFO_DEPTH.
-	 */
-	port->rx_rfr = port->rx_fifo_depth - 2;
-	port->rx_wm = UART_CONSOLE_RX_WM;
-	port->tx_wm = DEF_TX_WM;
-}
 
 static void qcom_geni_serial_shutdown(struct uart_port *uport)
 {
@@ -903,7 +887,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 
 	get_tx_fifo_size(port);
 
-	set_rfr_wm(port);
 	writel(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
 	/*
 	 * Make an unconditional cancel on the main sequencer to reset
@@ -916,7 +899,7 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 						false, true, false);
 	geni_se_config_packing(&port->se, BITS_PER_BYTE, port->rx_bytes_pw,
 						false, false, true);
-	geni_se_init(&port->se, port->rx_wm, port->rx_rfr);
+	geni_se_init(&port->se, UART_CONSOLE_RX_WM, port->rx_fifo_depth - 2);
 	geni_se_select_mode(&port->se, port->xfer_mode);
 	if (!uart_console(uport)) {
 		port->rx_fifo = devm_kcalloc(uport->dev,
-- 
2.20.1.415.g653613c723-goog

[PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable

[Ryan Case]

The driver only supports FIFO mode so setting and checking this variable
is unnecessary. If DMA support is ever addedd then such checks can be
introduced.

Signed-off-by: Ryan Case <ryandcase@chromium.org>
---

 drivers/tty/serial/qcom_geni_serial.c | 66 ++++++++++-----------------
 1 file changed, 24 insertions(+), 42 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 5521ed4a0708..3103aa0adc86 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -105,7 +105,6 @@ struct qcom_geni_serial_port {
 	u32 tx_fifo_depth;
 	u32 tx_fifo_width;
 	u32 rx_fifo_depth;
-	enum geni_se_xfer_mode xfer_mode;
 	bool setup;
 	int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
 	unsigned int baud;
@@ -555,29 +554,20 @@ static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
 static void qcom_geni_serial_start_tx(struct uart_port *uport)
 {
 	u32 irq_en;
-	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 	u32 status;
 
-	if (port->xfer_mode == GENI_SE_FIFO) {
-		/*
-		 * readl ensures reading & writing of IRQ_EN register
-		 * is not re-ordered before checking the status of the
-		 * Serial Engine.
-		 */
-		status = readl(uport->membase + SE_GENI_STATUS);
-		if (status & M_GENI_CMD_ACTIVE)
-			return;
+	status = readl(uport->membase + SE_GENI_STATUS);
+	if (status & M_GENI_CMD_ACTIVE)
+		return;
 
-		if (!qcom_geni_serial_tx_empty(uport))
-			return;
+	if (!qcom_geni_serial_tx_empty(uport))
+		return;
 
-		irq_en = readl(uport->membase +	SE_GENI_M_IRQ_EN);
-		irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
+	irq_en = readl(uport->membase +	SE_GENI_M_IRQ_EN);
+	irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
 
-		writel(DEF_TX_WM, uport->membase +
-						SE_GENI_TX_WATERMARK_REG);
-		writel(irq_en, uport->membase +	SE_GENI_M_IRQ_EN);
-	}
+	writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
+	writel(irq_en, uport->membase +	SE_GENI_M_IRQ_EN);
 }
 
 static void qcom_geni_serial_stop_tx(struct uart_port *uport)
@@ -588,11 +578,8 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
 
 	irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
 	irq_en &= ~M_CMD_DONE_EN;
-	if (port->xfer_mode == GENI_SE_FIFO) {
-		irq_en &= ~M_TX_FIFO_WATERMARK_EN;
-		writel(0, uport->membase +
-				     SE_GENI_TX_WATERMARK_REG);
-	}
+	irq_en &= ~M_TX_FIFO_WATERMARK_EN;
+	writel(0, uport->membase + SE_GENI_TX_WATERMARK_REG);
 	writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
 	status = readl(uport->membase + SE_GENI_STATUS);
 	/* Possible stop tx is called multiple times. */
@@ -623,15 +610,13 @@ static void qcom_geni_serial_start_rx(struct uart_port *uport)
 
 	geni_se_setup_s_cmd(&port->se, UART_START_READ, 0);
 
-	if (port->xfer_mode == GENI_SE_FIFO) {
-		irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
-		irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
-		writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+	irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
+	irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
+	writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
 
-		irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
-		irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
-		writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
-	}
+	irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+	irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
+	writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
 }
 
 static void qcom_geni_serial_stop_rx(struct uart_port *uport)
@@ -641,15 +626,13 @@ static void qcom_geni_serial_stop_rx(struct uart_port *uport)
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 	u32 irq_clear = S_CMD_DONE_EN;
 
-	if (port->xfer_mode == GENI_SE_FIFO) {
-		irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
-		irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
-		writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
+	irq_en = readl(uport->membase + SE_GENI_S_IRQ_EN);
+	irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
+	writel(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
 
-		irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
-		irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
-		writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
-	}
+	irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
+	irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
+	writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
 
 	status = readl(uport->membase + SE_GENI_STATUS);
 	/* Possible stop rx is called multiple times. */
@@ -892,7 +875,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 	 * Make an unconditional cancel on the main sequencer to reset
 	 * it else we could end up in data loss scenarios.
 	 */
-	port->xfer_mode = GENI_SE_FIFO;
 	if (uart_console(uport))
 		qcom_geni_serial_poll_tx_done(uport);
 	geni_se_config_packing(&port->se, BITS_PER_BYTE, port->tx_bytes_pw,
@@ -900,7 +882,7 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 	geni_se_config_packing(&port->se, BITS_PER_BYTE, port->rx_bytes_pw,
 						false, false, true);
 	geni_se_init(&port->se, UART_CONSOLE_RX_WM, port->rx_fifo_depth - 2);
-	geni_se_select_mode(&port->se, port->xfer_mode);
+	geni_se_select_mode(&port->se, GENI_SE_FIFO);
 	if (!uart_console(uport)) {
 		port->rx_fifo = devm_kcalloc(uport->dev,
 			port->rx_fifo_depth, sizeof(u32), GFP_KERNEL);
-- 
2.20.1.415.g653613c723-goog

[PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables

[Ryan Case]

Signed-off-by: Ryan Case <ryandcase@chromium.org>
---

 drivers/tty/serial/qcom_geni_serial.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3103aa0adc86..fa67a2ced420 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -765,12 +765,12 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
 
 static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 {
-	unsigned int m_irq_status;
-	unsigned int s_irq_status;
-	unsigned int geni_status;
+	u32 m_irq_en;
+	u32 m_irq_status;
+	u32 s_irq_status;
+	u32 geni_status;
 	struct uart_port *uport = dev;
 	unsigned long flags;
-	unsigned int m_irq_en;
 	bool drop_rx = false;
 	struct tty_port *tport = &uport->state->port;
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
@@ -948,14 +948,14 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 				struct ktermios *termios, struct ktermios *old)
 {
 	unsigned int baud;
-	unsigned int bits_per_char;
-	unsigned int tx_trans_cfg;
-	unsigned int tx_parity_cfg;
-	unsigned int rx_trans_cfg;
-	unsigned int rx_parity_cfg;
-	unsigned int stop_bit_len;
+	u32 bits_per_char;
+	u32 tx_trans_cfg;
+	u32 tx_parity_cfg;
+	u32 rx_trans_cfg;
+	u32 rx_parity_cfg;
+	u32 stop_bit_len;
 	unsigned int clk_div;
-	unsigned long ser_clk_cfg;
+	u32 ser_clk_cfg;
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 	unsigned long clk_rate;
 
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables

[Stephen Boyd]

Quoting Ryan Case (2019-01-02 13:36:36)
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---

It'd be nice to have any sort of commit text here with the motivation
for the change (code clarity is my motivation).

Besides that nitpick

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables

[Greg Kroah-Hartman]

On Wed, Jan 02, 2019 at 01:36:36PM -0800, Ryan Case wrote:
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
> 
>  drivers/tty/serial/qcom_geni_serial.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

I can not take patches without any changelog text, sorry.

greg k-h

Re: [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb()

[Evan Green]

On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> A frequent side comment has been to remove the use of writel_relaxed,
> readl_relaxed, and mb.

To provide a bit more motivation, you could add something to the
effect of "using the _relaxed variant introduces a significant amount
of complexity when reasoning about the code, and has not been shown to
provide a noticeable performance benefit in the case of this driver",
or something to that effect.

>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
>  drivers/tty/serial/qcom_geni_serial.c | 187 +++++++++++---------------
>  1 file changed, 80 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 2b1e73193dad..dc95b96148ed 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -230,7 +230,7 @@ static unsigned int qcom_geni_serial_get_mctrl(struct uart_port *uport)
>         if (uart_console(uport) || !uart_cts_enabled(uport)) {
>                 mctrl |= TIOCM_CTS;
>         } else {
> -               geni_ios = readl_relaxed(uport->membase + SE_GENI_IOS);
> +               geni_ios = readl(uport->membase + SE_GENI_IOS);
>                 if (!(geni_ios & IO2_DATA_IN))
>                         mctrl |= TIOCM_CTS;
>         }
> @@ -248,7 +248,7 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport,
>
>         if (!(mctrl & TIOCM_RTS))
>                 uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY;
> -       writel_relaxed(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
> +       writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR);
>  }
>
>  static const char *qcom_geni_serial_get_type(struct uart_port *uport)
> @@ -277,9 +277,6 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
>         unsigned int fifo_bits;
>         unsigned long timeout_us = 20000;
>
> -       /* Ensure polling is not re-ordered before the prior writes/reads */
> -       mb();
> -
>         if (uport->private_data) {
>                 port = to_dev_port(uport, uport);
>                 baud = port->baud;
> @@ -299,7 +296,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_relaxed(uport->membase + offset);
> +               reg = readl(uport->membase + offset);
>                 if ((bool)(reg & field) == set)
>                         return true;
>                 udelay(10);
> @@ -312,7 +309,7 @@ static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
>  {
>         u32 m_cmd;
>
> -       writel_relaxed(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
> +       writel(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
>         m_cmd = UART_START_TX << M_OPCODE_SHFT;
>         writel(m_cmd, uport->membase + SE_GENI_M_CMD0);
>  }
> @@ -325,13 +322,13 @@ static void qcom_geni_serial_poll_tx_done(struct uart_port *uport)
>         done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>                                                 M_CMD_DONE_EN, true);
>         if (!done) {
> -               writel_relaxed(M_GENI_CMD_ABORT, uport->membase +
> +               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_relaxed(irq_clear, 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)
> @@ -341,8 +338,8 @@ static void qcom_geni_serial_abort_rx(struct uart_port *uport)
>         writel(S_GENI_CMD_ABORT, uport->membase + SE_GENI_S_CMD_CTRL_REG);
>         qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
>                                         S_GENI_CMD_ABORT, false);
> -       writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
> -       writel_relaxed(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
> +       writel(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
> +       writel(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
>  }
>
>  #ifdef CONFIG_CONSOLE_POLL
> @@ -351,19 +348,13 @@ static int qcom_geni_serial_get_char(struct uart_port *uport)
>         u32 rx_fifo;
>         u32 status;
>
> -       status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
> -       writel_relaxed(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
> -
> -       status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
> -       writel_relaxed(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
> +       status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
> +       writel(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
>
> -       /*
> -        * Ensure the writes to clear interrupts is not re-ordered after
> -        * reading the data.
> -        */
> -       mb();
> +       status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
> +       writel(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
>
> -       status = readl_relaxed(uport->membase + SE_GENI_RX_FIFO_STATUS);
> +       status = readl(uport->membase + SE_GENI_RX_FIFO_STATUS);
>         if (!(status & RX_FIFO_WC_MSK))
>                 return NO_POLL_CHAR;
>
> @@ -376,12 +367,12 @@ static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
>  {
>         struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>
> -       writel_relaxed(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
> +       writel(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
>         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_relaxed(c, uport->membase + SE_GENI_TX_FIFOn);
> -       writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> +       writel(c, uport->membase + SE_GENI_TX_FIFOn);
> +       writel(M_TX_FIFO_WATERMARK_EN, uport->membase +
>                                                         SE_GENI_M_IRQ_CLEAR);

I think this could fit on one line now.

>         qcom_geni_serial_poll_tx_done(uport);
>  }
> @@ -390,7 +381,7 @@ static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
>  #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
>  static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
>  {
> -       writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn);
> +       writel(ch, uport->membase + SE_GENI_TX_FIFOn);
>  }
>
>  static void
> @@ -409,7 +400,7 @@ __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
>                         bytes_to_send++;
>         }
>
> -       writel_relaxed(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> +       writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
>         qcom_geni_serial_setup_tx(uport, bytes_to_send);
>         for (i = 0; i < count; ) {
>                 size_t chars_to_write = 0;
> @@ -427,7 +418,7 @@ __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
>                 chars_to_write = min_t(size_t, count - i, avail / 2);
>                 uart_console_write(uport, s + i, chars_to_write,
>                                                 qcom_geni_serial_wr_char);
> -               writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> +               writel(M_TX_FIFO_WATERMARK_EN, uport->membase +
>                                                         SE_GENI_M_IRQ_CLEAR);
>                 i += chars_to_write;
>         }
> @@ -456,7 +447,7 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
>         else
>                 spin_lock_irqsave(&uport->lock, flags);
>
> -       geni_status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> +       geni_status = readl(uport->membase + SE_GENI_STATUS);
>
>         /* Cancel the current write to log the fault */
>         if (!locked) {
> @@ -466,10 +457,10 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
>                         geni_se_abort_m_cmd(&port->se);
>                         qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>                                                         M_CMD_ABORT_EN, true);
> -                       writel_relaxed(M_CMD_ABORT_EN, uport->membase +
> +                       writel(M_CMD_ABORT_EN, uport->membase +
>                                                         SE_GENI_M_IRQ_CLEAR);
>                 }
> -               writel_relaxed(M_CMD_CANCEL_EN, uport->membase +
> +               writel(M_CMD_CANCEL_EN, uport->membase +
>                                                         SE_GENI_M_IRQ_CLEAR);

This one would also fit on one line now.

>         } else if ((geni_status & M_GENI_CMD_ACTIVE) && !port->tx_remaining) {
>                 /*
> @@ -479,9 +470,9 @@ static void qcom_geni_serial_console_write(struct console *co, const char *s,
>                 qcom_geni_serial_poll_tx_done(uport);
>
>                 if (uart_circ_chars_pending(&uport->state->xmit)) {
> -                       irq_en = readl_relaxed(uport->membase +
> +                       irq_en = readl(uport->membase +
>                                         SE_GENI_M_IRQ_EN);

This one too.

> -                       writel_relaxed(irq_en | M_TX_FIFO_WATERMARK_EN,
> +                       writel(irq_en | M_TX_FIFO_WATERMARK_EN,
>                                         uport->membase + SE_GENI_M_IRQ_EN);
>                 }
>         }
> @@ -585,12 +576,12 @@ static void qcom_geni_serial_start_tx(struct uart_port *uport)
>                 if (!qcom_geni_serial_tx_empty(uport))
>                         return;
>
> -               irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> +               irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
>                 irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
>
> -               writel_relaxed(port->tx_wm, uport->membase +
> +               writel(port->tx_wm, uport->membase +
>                                                 SE_GENI_TX_WATERMARK_REG);

This one too, though it looks like you fix that up in a later commit.

> -               writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> +               writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
>         }
>  }
>
> @@ -600,35 +591,29 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
>         u32 status;
>         struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>
> -       irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> +       irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
>         irq_en &= ~M_CMD_DONE_EN;
>         if (port->xfer_mode == GENI_SE_FIFO) {
>                 irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> -               writel_relaxed(0, uport->membase +
> +               writel(0, uport->membase +
>                                      SE_GENI_TX_WATERMARK_REG);

Here too, though it also appears fixed up by a later commit.

>         }
> -       writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> -       status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> +       writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> +       status = readl(uport->membase + SE_GENI_STATUS);
>         /* Possible stop tx is called multiple times. */
>         if (!(status & M_GENI_CMD_ACTIVE))
>                 return;
>
> -       /*
> -        * Ensure cancel command write is not re-ordered before checking
> -        * the status of the Primary Sequencer.
> -        */
> -       mb();
> -
>         geni_se_cancel_m_cmd(&port->se);
>         if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>                                                 M_CMD_CANCEL_EN, true)) {
>                 geni_se_abort_m_cmd(&port->se);
>                 qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
>                                                 M_CMD_ABORT_EN, true);
> -               writel_relaxed(M_CMD_ABORT_EN, uport->membase +
> +               writel(M_CMD_ABORT_EN, uport->membase +
>                                                         SE_GENI_M_IRQ_CLEAR);

Here too.

>         }
> -       writel_relaxed(M_CMD_CANCEL_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_start_rx(struct uart_port *uport)
> @@ -637,26 +622,20 @@ static void qcom_geni_serial_start_rx(struct uart_port *uport)
>         u32 status;
>         struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>
> -       status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> +       status = readl(uport->membase + SE_GENI_STATUS);
>         if (status & S_GENI_CMD_ACTIVE)
>                 qcom_geni_serial_stop_rx(uport);
>
> -       /*
> -        * Ensure setup command write is not re-ordered before checking
> -        * the status of the Secondary Sequencer.
> -        */
> -       mb();
> -

mmm, good deletes.

With the minor line coalescing fixed you can add my:

Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables

[Evan Green]

On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> The variables of tx_wm and rx_wm were set to the same define value in
> all cases, never updated, and the define was sometimes used
> interchangably. Remove the variables/function and use the fixed value.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
>  drivers/tty/serial/qcom_geni_serial.c | 23 +++--------------------
>  1 file changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index dc95b96148ed..5521ed4a0708 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -105,9 +105,6 @@ struct qcom_geni_serial_port {
>         u32 tx_fifo_depth;
>         u32 tx_fifo_width;
>         u32 rx_fifo_depth;
> -       u32 tx_wm;
> -       u32 rx_wm;
> -       u32 rx_rfr;
>         enum geni_se_xfer_mode xfer_mode;
>         bool setup;
>         int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
> @@ -365,9 +362,7 @@ 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)
>  {
> -       struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> -
> -       writel(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
> +       writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
>         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));
> @@ -579,7 +574,7 @@ static void qcom_geni_serial_start_tx(struct uart_port *uport)
>                 irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
>                 irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
>
> -               writel(port->tx_wm, uport->membase +
> +               writel(DEF_TX_WM, uport->membase +
>                                                 SE_GENI_TX_WATERMARK_REG);
>                 writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
>         }
> @@ -852,17 +847,6 @@ static void get_tx_fifo_size(struct qcom_geni_serial_port *port)
>                 (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
>  }
>
> -static void set_rfr_wm(struct qcom_geni_serial_port *port)
> -{
> -       /*
> -        * Set RFR (Flow off) to FIFO_DEPTH - 2.
> -        * RX WM level at 10% RX_FIFO_DEPTH.
> -        * TX WM level at 10% TX_FIFO_DEPTH.
> -        */
> -       port->rx_rfr = port->rx_fifo_depth - 2;
> -       port->rx_wm = UART_CONSOLE_RX_WM;
> -       port->tx_wm = DEF_TX_WM;
> -}
>
>  static void qcom_geni_serial_shutdown(struct uart_port *uport)
>  {
> @@ -903,7 +887,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>
>         get_tx_fifo_size(port);
>
> -       set_rfr_wm(port);
>         writel(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
>         /*
>          * Make an unconditional cancel on the main sequencer to reset
> @@ -916,7 +899,7 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>                                                 false, true, false);
>         geni_se_config_packing(&port->se, BITS_PER_BYTE, port->rx_bytes_pw,
>                                                 false, false, true);
> -       geni_se_init(&port->se, port->rx_wm, port->rx_rfr);
> +       geni_se_init(&port->se, UART_CONSOLE_RX_WM, port->rx_fifo_depth - 2);

It looks like the CONSOLE part of the name was never really correct,
since this is also used by the regular uart_ops as well. You could
optionally fold in a rename of this define in this change.

I was also trying to reason about why that - 2 was there, and if that
should be - UART_CONSOLE_RX_WM. But I don't really get why it's there,
so I can't say for sure that it's conceptually the same value. So I
guess that's fine as is.

Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable

[Evan Green]

On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> The driver only supports FIFO mode so setting and checking this variable
> is unnecessary. If DMA support is ever addedd then such checks can be

s/addedd/added/

> introduced.
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
>  drivers/tty/serial/qcom_geni_serial.c | 66 ++++++++++-----------------
>  1 file changed, 24 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 5521ed4a0708..3103aa0adc86 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -105,7 +105,6 @@ struct qcom_geni_serial_port {
>         u32 tx_fifo_depth;
>         u32 tx_fifo_width;
>         u32 rx_fifo_depth;
> -       enum geni_se_xfer_mode xfer_mode;
>         bool setup;
>         int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
>         unsigned int baud;
> @@ -555,29 +554,20 @@ static int handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
>  static void qcom_geni_serial_start_tx(struct uart_port *uport)
>  {
>         u32 irq_en;
> -       struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>         u32 status;
>
> -       if (port->xfer_mode == GENI_SE_FIFO) {
> -               /*
> -                * readl ensures reading & writing of IRQ_EN register
> -                * is not re-ordered before checking the status of the
> -                * Serial Engine.
> -                */
> -               status = readl(uport->membase + SE_GENI_STATUS);
> -               if (status & M_GENI_CMD_ACTIVE)
> -                       return;
> +       status = readl(uport->membase + SE_GENI_STATUS);
> +       if (status & M_GENI_CMD_ACTIVE)
> +               return;
>
> -               if (!qcom_geni_serial_tx_empty(uport))
> -                       return;
> +       if (!qcom_geni_serial_tx_empty(uport))
> +               return;
>
> -               irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> -               irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
> +       irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> +       irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
>
> -               writel(DEF_TX_WM, uport->membase +
> -                                               SE_GENI_TX_WATERMARK_REG);
> -               writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> -       }
> +       writel(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> +       writel(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
>  }
>
>  static void qcom_geni_serial_stop_tx(struct uart_port *uport)
> @@ -588,11 +578,8 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
>
>         irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
>         irq_en &= ~M_CMD_DONE_EN;
> -       if (port->xfer_mode == GENI_SE_FIFO) {
> -               irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> -               writel(0, uport->membase +
> -                                    SE_GENI_TX_WATERMARK_REG);
> -       }
> +       irq_en &= ~M_TX_FIFO_WATERMARK_EN;

This could be further coalesced into irq_en &= ~(M_CMD_DONE_EN |
M_TX_FIFO_WATERMARK_EN);

Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables

[Evan Green]

On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <ryandcase@chromium.org> wrote:
>
> Signed-off-by: Ryan Case <ryandcase@chromium.org>
> ---
>
>  drivers/tty/serial/qcom_geni_serial.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3103aa0adc86..fa67a2ced420 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -765,12 +765,12 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
>
>  static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
>  {
> -       unsigned int m_irq_status;
> -       unsigned int s_irq_status;
> -       unsigned int geni_status;
> +       u32 m_irq_en;
> +       u32 m_irq_status;
> +       u32 s_irq_status;
> +       u32 geni_status;
>         struct uart_port *uport = dev;
>         unsigned long flags;
> -       unsigned int m_irq_en;
>         bool drop_rx = false;
>         struct tty_port *tport = &uport->state->port;
>         struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> @@ -948,14 +948,14 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>                                 struct ktermios *termios, struct ktermios *old)
>  {
>         unsigned int baud;
> -       unsigned int bits_per_char;
> -       unsigned int tx_trans_cfg;
> -       unsigned int tx_parity_cfg;
> -       unsigned int rx_trans_cfg;
> -       unsigned int rx_parity_cfg;
> -       unsigned int stop_bit_len;
> +       u32 bits_per_char;
> +       u32 tx_trans_cfg;
> +       u32 tx_parity_cfg;
> +       u32 rx_trans_cfg;
> +       u32 rx_parity_cfg;
> +       u32 stop_bit_len;
>         unsigned int clk_div;
> -       unsigned long ser_clk_cfg;
> +       u32 ser_clk_cfg;
>         struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>         unsigned long clk_rate;
>
> --
> 2.20.1.415.g653613c723-goog
>

I did a brief search tour of unsigned in this file, and also found
rxstale in qcom_geni_serial_port_setup. Other than that and the commit
message:

Reviewed-by: Evan Green <evgreen@chromium.org>