[PATCH] tty: serial: qcom_geni_serial: Address follow-up comments

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



The driver has some follow-up comments right after it got merged. This
patch addresses those comments that got missed out.

* Document reason for newline character counting in console_write
* Document reason for disabling IRQ in the system resume operation
* Use min3 to find the minimum of 3 values
* Remove unnecessary casting while using min_t
* Use iowrite32_rep to write to the hardware FIFO
* Initialize the console port statically
* Fine-tune memory barrier usage

Signed-off-by: Karthikeyan Ramasubramanian <kramasub@xxxxxxxxxxxxxx>
---
 drivers/tty/serial/qcom_geni_serial.c | 84 ++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 40 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 1442777..f5b9cb8 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -105,7 +105,7 @@ struct qcom_geni_serial_port {
 	bool brk;
 };
 
-static const struct uart_ops qcom_geni_serial_pops;
+static const struct uart_ops qcom_geni_console_pops;
 static struct uart_driver qcom_geni_console_driver;
 static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop);
 static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port);
@@ -118,7 +118,14 @@ struct qcom_geni_serial_port {
 #define to_dev_port(ptr, member) \
 		container_of(ptr, struct qcom_geni_serial_port, member)
 
-static struct qcom_geni_serial_port qcom_geni_console_port;
+static struct qcom_geni_serial_port qcom_geni_console_port = {
+	.uport = {
+		.iotype = UPIO_MEM,
+		.ops = &qcom_geni_console_pops,
+		.flags = UPF_BOOT_AUTOCONF,
+		.line = 0,
+	},
+};
 
 static int qcom_geni_serial_request_port(struct uart_port *uport)
 {
@@ -173,8 +180,12 @@ 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();
+	/*
+	 * Ensure polling is not re-ordered before the prior writes/reads.
+	 * Just invoke the write memory barrier here, since readl_poll*
+	 * performs readl which invokes the read memory barrier.
+	 */
+	wmb();
 
 	if (uport->private_data) {
 		port = to_dev_port(uport, uport);
@@ -286,6 +297,10 @@ static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
 	u32 bytes_to_send = count;
 
 	for (i = 0; i < count; i++) {
+		/*
+		 * uart_console_write() adds a carriage return for each newline.
+		 * Account for additional bytes to be written.
+		 */
 		if (s[i] == '\n')
 			bytes_to_send++;
 	}
@@ -305,7 +320,7 @@ static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
 		if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
 						M_TX_FIFO_WATERMARK_EN, true))
 			break;
-		chars_to_write = min_t(size_t, (size_t)(count - i), avail / 2);
+		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 +
@@ -406,20 +421,18 @@ static void qcom_geni_serial_start_tx(struct uart_port *uport)
 	u32 status;
 
 	if (port->xfer_mode == GENI_SE_FIFO) {
-		status = readl_relaxed(uport->membase + SE_GENI_STATUS);
+		/*
+		 * 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;
 
 		if (!qcom_geni_serial_tx_empty(uport))
 			return;
 
-		/*
-		 * Ensure writing to IRQ_EN & watermark registers are not
-		 * re-ordered before checking the status of the Serial
-		 * Engine and TX FIFO
-		 */
-		mb();
-
 		irq_en = readl_relaxed(uport->membase +	SE_GENI_M_IRQ_EN);
 		irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
 
@@ -582,11 +595,7 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
 
 	avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
 	tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE - 1);
-	if (chunk > (UART_XMIT_SIZE - tail))
-		chunk = UART_XMIT_SIZE - tail;
-	if (chunk > avail)
-		chunk = avail;
-
+	chunk = min3((size_t)chunk, UART_XMIT_SIZE - tail, avail);
 	if (!chunk)
 		goto out_write_wakeup;
 
@@ -595,15 +604,15 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
 	remaining = chunk;
 	for (i = 0; i < chunk; ) {
 		unsigned int tx_bytes;
-		unsigned int buf = 0;
+		u8 buf[sizeof(u32)];
 		int c;
 
+		memset(buf, 0, ARRAY_SIZE(buf));
 		tx_bytes = min_t(size_t, remaining, (size_t)port->tx_bytes_pw);
 		for (c = 0; c < tx_bytes ; c++)
-			buf |= (xmit->buf[tail + c] << (c * BITS_PER_BYTE));
-
-		writel_relaxed(buf, uport->membase + SE_GENI_TX_FIFOn);
+			buf[c] = xmit->buf[tail + c];
 
+		iowrite32_rep(uport->membase + SE_GENI_TX_FIFOn, buf, 1);
 		i += tx_bytes;
 		tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
 		uport->icount.tx += tx_bytes;
@@ -627,7 +636,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
 
 	if (uport->suspended)
-		return IRQ_HANDLED;
+		return IRQ_NONE;
 
 	spin_lock_irqsave(&uport->lock, flags);
 	m_irq_status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
@@ -671,9 +680,6 @@ static int get_tx_fifo_size(struct qcom_geni_serial_port *port)
 {
 	struct uart_port *uport;
 
-	if (!port)
-		return -ENODEV;
-
 	uport = &port->uport;
 	port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se);
 	port->tx_fifo_width = geni_se_get_tx_fifo_width(&port->se);
@@ -702,7 +708,6 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
 	/* Stop the console before stopping the current tx */
 	console_stop(uport->cons);
 
-	disable_irq(uport->irq);
 	free_irq(uport->irq, uport);
 	spin_lock_irqsave(&uport->lock, flags);
 	qcom_geni_serial_stop_tx(uport);
@@ -892,7 +897,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
 
 static unsigned int qcom_geni_serial_tx_empty(struct uart_port *uport)
 {
-	return !readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
+	return !readl(uport->membase + SE_GENI_TX_FIFO_STATUS);
 }
 
 #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
@@ -910,7 +915,7 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
 
 	port = get_port_from_line(co->index);
 	if (IS_ERR(port)) {
-		pr_err("Invalid line %d(%d)\n", co->index, (int)PTR_ERR(port));
+		pr_err("Invalid line %d\n", co->index);
 		return PTR_ERR(port);
 	}
 
@@ -1025,16 +1030,13 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 
 	if (pdev->dev.of_node)
 		line = of_alias_get_id(pdev->dev.of_node, "serial");
-	else
-		line = pdev->id;
 
 	if (line < 0 || line >= GENI_UART_CONS_PORTS)
 		return -ENXIO;
 	port = get_port_from_line(line);
 	if (IS_ERR(port)) {
-		ret = PTR_ERR(port);
-		dev_err(&pdev->dev, "Invalid line %d(%d)\n", line, ret);
-		return ret;
+		dev_err(&pdev->dev, "Invalid line %d\n", line);
+		return PTR_ERR(port);
 	}
 
 	uport = &port->uport;
@@ -1070,7 +1072,6 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	uport->private_data = &qcom_geni_console_driver;
 	platform_set_drvdata(pdev, port);
 	port->handle_rx = handle_rx_console;
-	port->setup = false;
 	return uart_add_one_port(&qcom_geni_console_driver, uport);
 }
 
@@ -1101,6 +1102,14 @@ static int __maybe_unused qcom_geni_serial_sys_resume_noirq(struct device *dev)
 
 	if (console_suspend_enabled && uport->suspended) {
 		uart_resume_port(uport->private_data, uport);
+		/*
+		 * uart_suspend_port() invokes port shutdown which in turn
+		 * frees the irq. uart_resume_port invokes port startup which
+		 * performs request_irq. The request_irq auto-enables the IRQ.
+		 * In addition, resume_noirq implicitly enables the IRQ and
+		 * leads to an unbalanced IRQ enable warning. Disable the IRQ
+		 * before returning so that the warning is suppressed.
+		 */
 		disable_irq(uport->irq);
 	}
 	return 0;
@@ -1131,11 +1140,6 @@ static int __init qcom_geni_serial_init(void)
 {
 	int ret;
 
-	qcom_geni_console_port.uport.iotype = UPIO_MEM;
-	qcom_geni_console_port.uport.ops = &qcom_geni_console_pops;
-	qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF;
-	qcom_geni_console_port.uport.line = 0;
-
 	ret = console_register(&qcom_geni_console_driver);
 	if (ret)
 		return ret;
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux