diff mbox

tty: serial: qcom_geni_serial: Address follow-up comments

Message ID 1521843038-19903-1-git-send-email-kramasub@codeaurora.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Karthikeyan Ramasubramanian March 23, 2018, 10:10 p.m. UTC
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@codeaurora.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 84 ++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 40 deletions(-)

Comments

Evan Green April 3, 2018, 7:15 p.m. UTC | #1
Hi Karthik,

On Fri, Mar 23, 2018 at 3:10 PM Karthikeyan Ramasubramanian <
kramasub@codeaurora.org> wrote:

> 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@codeaurora.org>
> ---
>   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
...
> @@ -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;


Thanks for doing this fixup in start_tx. The same pattern would apply in
stop_tx, start_rx, and stop_rx, right?

-Evan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karthikeyan Ramasubramanian April 4, 2018, 1:42 p.m. UTC | #2
On 4/3/2018 1:15 PM, Evan Green wrote:
> Hi Karthik,
> 
> On Fri, Mar 23, 2018 at 3:10 PM Karthikeyan Ramasubramanian <
> kramasub@codeaurora.org> wrote:
> 
>> 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@codeaurora.org>
>> ---
>>   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
> ...
>> @@ -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;
> 
> 
> Thanks for doing this fixup in start_tx. The same pattern would apply in
> stop_tx, start_rx, and stop_rx, right?
The read barrier coupled with the data dependency ensures that the write
to IRQ_EN is not re-ordered ahead of checking the GENI_STATUS. The write
to watermark register can still be re-ordered but it does not cause any
impact.

In the other cases, the cancel command is written to a different
register. There is no data dependency and hence if cancel command write
gets reordered ahead of checking the GENI_STATUS, then the code may
return prematurely before some clean-up is done.
> 
> -Evan
> 
Regards,
Karthik.
Fabio Estevam April 4, 2018, 1:47 p.m. UTC | #3
On Fri, Mar 23, 2018 at 7:10 PM, Karthikeyan Ramasubramanian
<kramasub@codeaurora.org> wrote:
> 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

Too many changes for a single patch. Please split the changes into
individual patches.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Karthikeyan Ramasubramanian April 4, 2018, 2:02 p.m. UTC | #4
On 4/4/2018 7:47 AM, Fabio Estevam wrote:
> On Fri, Mar 23, 2018 at 7:10 PM, Karthikeyan Ramasubramanian
> <kramasub@codeaurora.org> wrote:
>> 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
> 
> Too many changes for a single patch. Please split the changes into
> individual patches.
> 
Ok, I will split the changes into individual patches and upload a patch
series.

Regards,
Karthik.
diff mbox

Patch

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;