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
Signed-off-by: Ryan Case <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
A frequent side comment has been to remove the use of writel_relaxed,
readl_relaxed, and mb.
Signed-off-by: Ryan Case <[email protected]>
---
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
Quoting Ryan Case (2019-01-02 13:36:36)
> Signed-off-by: Ryan Case <[email protected]>
> ---
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 <[email protected]>
On Wed, Jan 02, 2019 at 01:36:36PM -0800, Ryan Case wrote:
> Signed-off-by: Ryan Case <[email protected]>
> ---
>
> 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
On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <[email protected]> wrote:
>
> Signed-off-by: Ryan Case <[email protected]>
> ---
>
> 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 <[email protected]>
On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
On Wed, Jan 2, 2019 at 1:37 PM Ryan Case <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]>