Re: [PATCH] serial 8250: tighten test for using backup timer

["Will Newton" <will.newton@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4331 bytes --]

On Tue, Aug 5, 2008 at 9:06 PM, Alex Williamson <alex.williamson@hp.com> wrote:
> Hi Will,
>
> On Tue, 2008-08-05 at 12:44 +0100, Will Newton wrote:
>> Hi,
>>
>> Sorry to not have picked up on this earlier, but this change seems to
>> break an old DesignWare UART I have in an SoC here. It has a number of
>> known issues, one of which is it does not appropriately reassert THRE.
>>
>> What seems to happen is that the first time the port is opened the
>> code tests for incorrect reassertion of THRE and correctly sets up the
>> backup timer. The port is closed and subsequently reopened and this
>> time around the new logic prevents the backup timer from being
>> enabled. I'm not 100% sure of the details of the bug that is being
>> worked around here, but it appears that the second time the port is
>> opened it is not possible to detect the bug because the previous THRE
>> condition has already been acknowledged.
>>
>> The attached patch fixes the problem for me and attempts to preserve
>> the new behaviour at the same time. Comments?
>
> It would be nice if there was a simple procedure we could do on the UART
> to reset it to a state so the test works repeatedly.  However, with all
> the buggy UARTs out there, that could end up disturbing someone else.
>
> This change works for me, though it is using up a bit in the bugs field;
> not that we seem to be allocating them at any great rate.  I think it
> would be worthy of a comment in the code to understand why this new
> block exists outside the test so we don't need to refer back to the
> commit changeset.
>
> Acked-by: Alex Williamson <alex.williamson@hp.com>

I've added a comment and your ack, thanks.

Andrew: Are you the appropriate person to send patches for the serial
subsystem to?

>From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001
From: Will Newton <will.newton@gmail.com>
Date: Wed, 6 Aug 2008 11:48:29 +0100
Subject: [PATCH] 8250: Improve workaround for UARTs that don't
re-assert THRE correctly.

Recent changes to tighten the check for UARTs that don't correctly
re-assert THRE caused problems when such a UART was opened for the second
time - the bug could only successfully be detected at first initialization.
This patch stores the information about the bug in the bugs field of the
port structure when the port is first started up so subsequent opens can
check this bit even if the test for the bug fails.

Signed-off-by: Will Newton <will.newton@gmail.com>
Acked-by: Alex Williamson <alex.williamson@hp.com>

---
 drivers/serial/8250.c |   16 ++++++++++++----
 drivers/serial/8250.h |    1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 342e12f..9ccc563 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1908,15 +1908,23 @@ static int serial8250_startup(struct uart_port *port)
 		 * kick the UART on a regular basis.
 		 */
 		if (!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) {
+			up->bugs |= UART_BUG_THRE;
 			pr_debug("ttyS%d - using backup timer\n", port->line);
-			up->timer.function = serial8250_backup_timeout;
-			up->timer.data = (unsigned long)up;
-			mod_timer(&up->timer, jiffies +
-				poll_timeout(up->port.timeout) + HZ / 5);
 		}
 	}

 	/*
+	 * The above check will only give an accurate result the first time
+	 * the port is opened so this value needs to be preserved.
+	 */
+	if (up->bugs & UART_BUG_THRE) {
+		up->timer.function = serial8250_backup_timeout;
+		up->timer.data = (unsigned long)up;
+		mod_timer(&up->timer, jiffies +
+			  poll_timeout(up->port.timeout) + HZ / 5);
+	}
+
+	/*
 	 * If the "interrupt" for this port doesn't correspond with any
 	 * hardware interrupt, we use a timer-based system.  The original
 	 * driver used to do this with IRQ0.
diff --git a/drivers/serial/8250.h b/drivers/serial/8250.h
index 78c0016..5202603 100644
--- a/drivers/serial/8250.h
+++ b/drivers/serial/8250.h
@@ -47,6 +47,7 @@ struct serial8250_config {
 #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
 #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
 #define UART_BUG_NOMSR	(1 << 2)	/* UART has buggy MSR status bits (Au1x00) */
+#define UART_BUG_THRE	(1 << 3)	/* UART has buggy THRE reassertion */

 #define PROBE_RSA	(1 << 0)
 #define PROBE_ANY	(~0)
-- 
1.5.5.2

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-8250-Improve-workaround-for-UARTs-that-don-t-re-ass.patch --]
[-- Type: text/x-patch; name=0001-8250-Improve-workaround-for-UARTs-that-don-t-re-ass.patch, Size: 2570 bytes --]

From 36ac82a231498247ada098d31e8d12e735eb34f2 Mon Sep 17 00:00:00 2001
From: Will Newton <will.newton@gmail.com>
Date: Wed, 6 Aug 2008 11:48:29 +0100
Subject: [PATCH] 8250: Improve workaround for UARTs that don't re-assert THRE correctly.

Recent changes to tighten the check for UARTs that don't correctly
re-assert THRE caused problems when such a UART was opened for the second
time - the bug could only successfully be detected at first initialization.
This patch stores the information about the bug in the bugs field of the
port structure when the port is first started up so subsequent opens can
check this bit even if the test for the bug fails.

Signed-off-by: Will Newton <will.newton@gmail.com>
Acked-by: Alex Williamson <alex.williamson@hp.com>

---
 drivers/serial/8250.c |   16 ++++++++++++----
 drivers/serial/8250.h |    1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 342e12f..9ccc563 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1908,15 +1908,23 @@ static int serial8250_startup(struct uart_port *port)
 		 * kick the UART on a regular basis.
 		 */
 		if (!(iir1 & UART_IIR_NO_INT) && (iir & UART_IIR_NO_INT)) {
+			up->bugs |= UART_BUG_THRE;
 			pr_debug("ttyS%d - using backup timer\n", port->line);
-			up->timer.function = serial8250_backup_timeout;
-			up->timer.data = (unsigned long)up;
-			mod_timer(&up->timer, jiffies +
-				poll_timeout(up->port.timeout) + HZ / 5);
 		}
 	}
 
 	/*
+	 * The above check will only give an accurate result the first time
+	 * the port is opened so this value needs to be preserved.
+	 */
+	if (up->bugs & UART_BUG_THRE) {
+		up->timer.function = serial8250_backup_timeout;
+		up->timer.data = (unsigned long)up;
+		mod_timer(&up->timer, jiffies +
+			  poll_timeout(up->port.timeout) + HZ / 5);
+	}
+
+	/*
 	 * If the "interrupt" for this port doesn't correspond with any
 	 * hardware interrupt, we use a timer-based system.  The original
 	 * driver used to do this with IRQ0.
diff --git a/drivers/serial/8250.h b/drivers/serial/8250.h
index 78c0016..5202603 100644
--- a/drivers/serial/8250.h
+++ b/drivers/serial/8250.h
@@ -47,6 +47,7 @@ struct serial8250_config {
 #define UART_BUG_QUOT	(1 << 0)	/* UART has buggy quot LSB */
 #define UART_BUG_TXEN	(1 << 1)	/* UART has buggy TX IIR status */
 #define UART_BUG_NOMSR	(1 << 2)	/* UART has buggy MSR status bits (Au1x00) */
+#define UART_BUG_THRE	(1 << 3)	/* UART has buggy THRE reassertion */
 
 #define PROBE_RSA	(1 << 0)
 #define PROBE_ANY	(~0)
-- 
1.5.5.2