* [PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups
@ 2019-01-02 21:36 Ryan Case
2019-01-02 21:36 ` [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() Ryan Case
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Ryan Case @ 2019-01-02 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial,
Stephen Boyd, 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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() 2019-01-02 21:36 [PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups Ryan Case @ 2019-01-02 21:36 ` Ryan Case 2019-01-04 19:03 ` Evan Green 2019-01-02 21:36 ` [PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables Ryan Case ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Ryan Case @ 2019-01-02 21:36 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() 2019-01-02 21:36 ` [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() Ryan Case @ 2019-01-04 19:03 ` Evan Green 0 siblings, 0 replies; 11+ messages in thread From: Evan Green @ 2019-01-04 19:03 UTC (permalink / raw) To: Ryan Case Cc: Greg Kroah-Hartman, Jiri Slaby, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables 2019-01-02 21:36 [PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups Ryan Case 2019-01-02 21:36 ` [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() Ryan Case @ 2019-01-02 21:36 ` Ryan Case 2019-01-04 19:04 ` Evan Green 2019-01-02 21:36 ` [PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable Ryan Case 2019-01-02 21:36 ` [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables Ryan Case 3 siblings, 1 reply; 11+ messages in thread From: Ryan Case @ 2019-01-02 21:36 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables 2019-01-02 21:36 ` [PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables Ryan Case @ 2019-01-04 19:04 ` Evan Green 0 siblings, 0 replies; 11+ messages in thread From: Evan Green @ 2019-01-04 19:04 UTC (permalink / raw) To: Ryan Case Cc: Greg Kroah-Hartman, Jiri Slaby, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable 2019-01-02 21:36 [PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups Ryan Case 2019-01-02 21:36 ` [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() Ryan Case 2019-01-02 21:36 ` [PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables Ryan Case @ 2019-01-02 21:36 ` Ryan Case 2019-01-04 19:04 ` Evan Green 2019-01-02 21:36 ` [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables Ryan Case 3 siblings, 1 reply; 11+ messages in thread From: Ryan Case @ 2019-01-02 21:36 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable 2019-01-02 21:36 ` [PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable Ryan Case @ 2019-01-04 19:04 ` Evan Green 0 siblings, 0 replies; 11+ messages in thread From: Evan Green @ 2019-01-04 19:04 UTC (permalink / raw) To: Ryan Case Cc: Greg Kroah-Hartman, Jiri Slaby, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables 2019-01-02 21:36 [PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups Ryan Case ` (2 preceding siblings ...) 2019-01-02 21:36 ` [PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable Ryan Case @ 2019-01-02 21:36 ` Ryan Case 2019-01-03 21:22 ` Stephen Boyd ` (2 more replies) 3 siblings, 3 replies; 11+ messages in thread From: Ryan Case @ 2019-01-02 21:36 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables 2019-01-02 21:36 ` [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables Ryan Case @ 2019-01-03 21:22 ` Stephen Boyd 2019-01-04 8:45 ` Greg Kroah-Hartman 2019-01-04 19:05 ` Evan Green 2 siblings, 0 replies; 11+ messages in thread From: Stephen Boyd @ 2019-01-03 21:22 UTC (permalink / raw) To: Greg Kroah-Hartman, Jiri Slaby Cc: Evan Green, Doug Anderson, linux-kernel, linux-serial, Ryan Case 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables 2019-01-02 21:36 ` [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables Ryan Case 2019-01-03 21:22 ` Stephen Boyd @ 2019-01-04 8:45 ` Greg Kroah-Hartman 2019-01-04 19:05 ` Evan Green 2 siblings, 0 replies; 11+ messages in thread From: Greg Kroah-Hartman @ 2019-01-04 8:45 UTC (permalink / raw) To: Ryan Case Cc: Jiri Slaby, Evan Green, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables 2019-01-02 21:36 ` [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables Ryan Case 2019-01-03 21:22 ` Stephen Boyd 2019-01-04 8:45 ` Greg Kroah-Hartman @ 2019-01-04 19:05 ` Evan Green 2 siblings, 0 replies; 11+ messages in thread From: Evan Green @ 2019-01-04 19:05 UTC (permalink / raw) To: Ryan Case Cc: Greg Kroah-Hartman, Jiri Slaby, Doug Anderson, linux-kernel, linux-serial, Stephen Boyd 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-01-04 19:05 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-01-02 21:36 [PATCH 0/4] tty: serial: qcom_geni_serial: Assorted cleanups Ryan Case 2019-01-02 21:36 ` [PATCH 1/4] tty: serial: qcom_geni_serial: Remove use of *_relaxed() and mb() Ryan Case 2019-01-04 19:03 ` Evan Green 2019-01-02 21:36 ` [PATCH 2/4] tty: serial: qcom_geni_serial: Remove set_rfr_wm() and related variables Ryan Case 2019-01-04 19:04 ` Evan Green 2019-01-02 21:36 ` [PATCH 3/4] tty: serial: qcom_geni_serial: Remove xfer_mode variable Ryan Case 2019-01-04 19:04 ` Evan Green 2019-01-02 21:36 ` [PATCH 4/4] tty: serial: qcom_geni_serial: Use u32 for register variables Ryan Case 2019-01-03 21:22 ` Stephen Boyd 2019-01-04 8:45 ` Greg Kroah-Hartman 2019-01-04 19:05 ` Evan Green
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).