[PATCH 0/3] RFC Fix Fsl 8250 BRK bug via letting plat code set bugs

[PATCH 0/3] RFC Fix Fsl 8250 BRK bug via letting plat code set bugs

[Paul Gortmaker]

This is a respin of an earlier patch[1] that enabled a workaround
via Kconfig for an errata issue when using BRK on FSL 16550 UARTs.

In this version, the 8250 specific bug field is moved to the generic
uart_port struct, since hardware bugs aren't the unique domain of 
just the 8250 class of uarts.

Then the ability to set a known bugs value via the platform_device entry
point is added, so that it can be set externally vs. requiring some sort
of autodetection from the actual driver(s).

Then, the FSL errata fix is added to 8250.c by using the above support.
An entry in a DTS file is used to flag boards/platforms with the issue.
It is also added in a way that is optimized away for other architectures.

Kumar has a patch pending[2] that updates all the dts files.  I've not
included it here, so that review focus can be on the implementation.

I've re-tested it on an sbc8641D, where sysrq was useless before; with
this fix, it works as expected.

Thanks,
Paul.

[1] http://patchwork.ozlabs.org/patch/46609/
[2] http://patchwork.ozlabs.org/patch/128070/

--------

Paul Gortmaker (3):
  serial: make bugs field not specific to 8250 type uarts.
  serial: allow passing in hardware bug info via platform device
  8250: add workaround for MPC8[356]xx UART break IRQ storm

 arch/powerpc/kernel/legacy_serial.c |   11 ++++++++++
 drivers/tty/serial/8250.c           |   37 +++++++++++++++++++++-------------
 drivers/tty/serial/8250.h           |    5 ----
 include/linux/serial_8250.h         |   11 ++++++++++
 include/linux/serial_core.h         |    1 +
 5 files changed, 46 insertions(+), 19 deletions(-)

-- 
1.7.7

[PATCH 1/3] serial: make bugs field not specific to 8250 type uarts.

[Paul Gortmaker]

There is a struct uart_8250_port that is private to 8250.c
itself, and it had a bugs field.  But there is no reason to
assume that hardware bugs are unique to 8250 type UARTS.

Make the bugs field part of the globally visible struct
uart_port and remove the 8250 specific one.

The value in doing so is that it helps pave the way for
allowing arch or platform specific code to pass in information
to the specific uart drivers about uart bugs known to impact
certain platforms that would otherwise be hard to detect from
within the context of the driver itself.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/8250.c   |   25 ++++++++++++-------------
 include/linux/serial_core.h |    1 +
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index eeadf1b..7c94dbc 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -134,7 +134,6 @@ struct uart_8250_port {
 	struct timer_list	timer;		/* "no irq" timer */
 	struct list_head	list;		/* ports on this IRQ */
 	unsigned short		capabilities;	/* port capabilities */
-	unsigned short		bugs;		/* port bugs */
 	unsigned int		tx_loadsz;	/* transmit fifo load size */
 	unsigned char		acr;
 	unsigned char		ier;
@@ -828,7 +827,7 @@ static void autoconfig_has_efr(struct uart_8250_port *up)
 		 * when DLL is 0.
 		 */
 		if (id3 == 0x52 && rev == 0x01)
-			up->bugs |= UART_BUG_QUOT;
+			up->port.bugs |= UART_BUG_QUOT;
 		return;
 	}
 
@@ -1102,7 +1101,7 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 	spin_lock_irqsave(&up->port.lock, flags);
 
 	up->capabilities = 0;
-	up->bugs = 0;
+	up->port.bugs = 0;
 
 	if (!(up->port.flags & UPF_BUGGY_UART)) {
 		/*
@@ -1337,7 +1336,7 @@ static void serial8250_start_tx(struct uart_port *port)
 		up->ier |= UART_IER_THRI;
 		serial_out(up, UART_IER, up->ier);
 
-		if (up->bugs & UART_BUG_TXEN) {
+		if (port->bugs & UART_BUG_TXEN) {
 			unsigned char lsr;
 			lsr = serial_in(up, UART_LSR);
 			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
@@ -1373,7 +1372,7 @@ static void serial8250_enable_ms(struct uart_port *port)
 		container_of(port, struct uart_8250_port, port);
 
 	/* no MSR capabilities */
-	if (up->bugs & UART_BUG_NOMSR)
+	if (port->bugs & UART_BUG_NOMSR)
 		return;
 
 	up->ier |= UART_IER_MSI;
@@ -2089,7 +2088,7 @@ 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;
+			port->bugs |= UART_BUG_THRE;
 			pr_debug("ttyS%d - using backup timer\n",
 				 serial_index(port));
 		}
@@ -2099,7 +2098,7 @@ static int serial8250_startup(struct uart_port *port)
 	 * 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) {
+	if (port->bugs & UART_BUG_THRE) {
 		up->timer.function = serial8250_backup_timeout;
 		up->timer.data = (unsigned long)up;
 		mod_timer(&up->timer, jiffies +
@@ -2162,13 +2161,13 @@ static int serial8250_startup(struct uart_port *port)
 	serial_outp(up, UART_IER, 0);
 
 	if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) {
-		if (!(up->bugs & UART_BUG_TXEN)) {
-			up->bugs |= UART_BUG_TXEN;
+		if (!(port->bugs & UART_BUG_TXEN)) {
+			port->bugs |= UART_BUG_TXEN;
 			pr_debug("ttyS%d - enabling bad tx status workarounds\n",
 				 serial_index(port));
 		}
 	} else {
-		up->bugs &= ~UART_BUG_TXEN;
+		port->bugs &= ~UART_BUG_TXEN;
 	}
 
 dont_test_tx_en:
@@ -2323,7 +2322,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	/*
 	 * Oxford Semi 952 rev B workaround
 	 */
-	if (up->bugs & UART_BUG_QUOT && (quot & 0xff) == 0)
+	if (port->bugs & UART_BUG_QUOT && (quot & 0xff) == 0)
 		quot++;
 
 	if (up->capabilities & UART_CAP_FIFO && up->port.fifosize > 1) {
@@ -2390,7 +2389,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	 * CTS flow control flag and modem status interrupts
 	 */
 	up->ier &= ~UART_IER_MSI;
-	if (!(up->bugs & UART_BUG_NOMSR) &&
+	if (!(port->bugs & UART_BUG_NOMSR) &&
 			UART_ENABLE_MS(&up->port, termios->c_cflag))
 		up->ier |= UART_IER_MSI;
 	if (up->capabilities & UART_CAP_UUE)
@@ -2666,7 +2665,7 @@ static void serial8250_config_port(struct uart_port *port, int flags)
 
 	/* if access method is AU, it is a 16550 with a quirk */
 	if (up->port.type == PORT_16550A && up->port.iotype == UPIO_AU)
-		up->bugs |= UART_BUG_NOMSR;
+		port->bugs |= UART_BUG_NOMSR;
 
 	if (up->port.type != PORT_UNKNOWN && flags & UART_CONFIG_IRQ)
 		autoconfig_irq(up);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index eadf33d..791536c 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -323,6 +323,7 @@ struct uart_port {
 
 	unsigned int		read_status_mask;	/* driver specific */
 	unsigned int		ignore_status_mask;	/* driver specific */
+	unsigned short		bugs;			/* driver specific */
 	struct uart_state	*state;			/* pointer to parent state */
 	struct uart_icount	icount;			/* statistics */
 
-- 
1.7.7

Re: [PATCH 1/3] serial: make bugs field not specific to 8250 type uarts.

[Alan Cox]

> Make the bugs field part of the globally visible struct
> uart_port and remove the 8250 specific one.

Except all the bits in it are 8250 specific things or names that are
meaningless in generic form - no. I also don't want to encourage flags
and bug bits. We already have too many and its making the code a mess.
So right now we want less not more.

Re: [PATCH 1/3] serial: make bugs field not specific to 8250 type uarts.

[Paul Gortmaker]

On Thu, Dec 1, 2011 at 7:51 PM, Alan Cox <alan@linux.intel.com> wrote:
>> Make the bugs field part of the globally visible struct
>> uart_port and remove the 8250 specific one.
>
> Except all the bits in it are 8250 specific things or names that are
> meaningless in generic form - no. I also don't want to encourage flags
> and bug bits. We already have too many and its making the code a mess.
> So right now we want less not more.

The bits in "bugs" are only passed through -- the idea is that there is
no "interpretation" of them by any generic layer -- only that they prop
from the arch down to the driver which knows what they mean.

I can understand the "want less not more" mentality -- I was thinking
the same thing when I was looking at the replication of fields between
uart_port and uart_8250_port, and toying with the idea of helping clean
that up....  (separate topic, to be sure.)

If you have an idea in mind how arch/platform code should cleanly
pass data about known uart bugs to the uart driver, then let me know
what you have in mind.  I've no real attachment to what I proposed
here -- it just happened to be the solution I thought would be the least
offensive.  If there is a better idea floating around, I'll go ahead and
try to implement it.

Thanks,
Paul.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" i=
n
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] serial: make bugs field not specific to 8250 type uarts.

[Alan Cox]

> If you have an idea in mind how arch/platform code should cleanly
> pass data about known uart bugs to the uart driver, then let me know
> what you have in mind.  I've no real attachment to what I proposed
> here -- it just happened to be the solution I thought would be the least
> offensive.  If there is a better idea floating around, I'll go ahead and
> try to implement it.

My first guess is that "the uart driver" is our first historical mistake.
Non standard 8250ish uarts should be their own platform driver which uses
8250.c for support.

Things like 8250_dw.c is perhaps more the style we want to be looking at
for other devices and adjusting 8250.c as needed to export or split
methods up so they can be used as helpers.

Alan

[PATCH 2/3] serial: allow passing in hardware bug info via platform device

[Paul Gortmaker]

The normal arch hook into the 8250 serial world is via passing
in a plat_serial8250_port struct.  However, this struct does
not have a bugs field, so there is no way to have the arch
code pass in info about known uart issues.

Add a bug field to the plat_serial8250_port struct, so that the
arch can pass in this information.  Also don't do a blanket
overwrite of the bugs setting in the 8250.c driver.  Finally,
relocate the known bug #define list to a globally visible header
so that the arch can assign any appropriate values from the list.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/8250.c   |    3 ++-
 drivers/tty/serial/8250.h   |    5 -----
 include/linux/serial_8250.h |    6 ++++++
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 7c94dbc..f99f27c 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -1101,7 +1101,6 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 	spin_lock_irqsave(&up->port.lock, flags);
 
 	up->capabilities = 0;
-	up->port.bugs = 0;
 
 	if (!(up->port.flags & UPF_BUGGY_UART)) {
 		/*
@@ -3075,6 +3074,7 @@ static int __devinit serial8250_probe(struct platform_device *dev)
 		port.serial_out		= p->serial_out;
 		port.handle_irq		= p->handle_irq;
 		port.set_termios	= p->set_termios;
+		port.bugs		= p->bugs;
 		port.pm			= p->pm;
 		port.dev		= &dev->dev;
 		port.irqflags		|= irqflag;
@@ -3225,6 +3225,7 @@ int serial8250_register_port(struct uart_port *port)
 		uart->port.regshift     = port->regshift;
 		uart->port.iotype       = port->iotype;
 		uart->port.flags        = port->flags | UPF_BOOT_AUTOCONF;
+		uart->port.bugs         = port->bugs;
 		uart->port.mapbase      = port->mapbase;
 		uart->port.private_data = port->private_data;
 		if (port->dev)
diff --git a/drivers/tty/serial/8250.h b/drivers/tty/serial/8250.h
index 6edf4a6..caefe00 100644
--- a/drivers/tty/serial/8250.h
+++ b/drivers/tty/serial/8250.h
@@ -44,11 +44,6 @@ struct serial8250_config {
 #define UART_CAP_UUE	(1 << 12)	/* UART needs IER bit 6 set (Xscale) */
 #define UART_CAP_RTOIE	(1 << 13)	/* UART needs IER bit 4 set (Xscale, Tegra) */
 
-#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)
 
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 1f05bbe..8c660af 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -14,6 +14,11 @@
 #include <linux/serial_core.h>
 #include <linux/platform_device.h>
 
+#define UART_BUG_QUOT	(1 << 0)	/* buggy quot LSB */
+#define UART_BUG_TXEN	(1 << 1)	/* buggy TX IIR status */
+#define UART_BUG_NOMSR	(1 << 2)	/* buggy MSR status bits (Au1x00) */
+#define UART_BUG_THRE	(1 << 3)	/* buggy THRE reassertion */
+
 /*
  * This is the platform device platform_data structure
  */
@@ -30,6 +35,7 @@ struct plat_serial8250_port {
 	unsigned char	hub6;
 	upf_t		flags;		/* UPF_* flags */
 	unsigned int	type;		/* If UPF_FIXED_TYPE */
+	unsigned short	bugs;		/* hardware specific bugs */
 	unsigned int	(*serial_in)(struct uart_port *, int);
 	void		(*serial_out)(struct uart_port *, int, int);
 	void		(*set_termios)(struct uart_port *,
-- 
1.7.7

[PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Paul Gortmaker]

Sending a break on the SOC UARTs found in some MPC83xx/85xx/86xx
chips seems to cause a short lived IRQ storm (/proc/interrupts
typically shows somewhere between 300 and 1500 events).  Unfortunately
this renders SysRQ over the serial console completely inoperable.

The suggested workaround in the errata is to read the Rx register,
wait one character period, and then read the Rx register again.
We achieve this by tracking the old LSR value, and on the subsequent
interrupt event after a break, we don't read LSR, instead we just
read the RBR again and return immediately.

The "fsl,ns16550" is used in the compatible field of the serial
device to mark UARTs known to have this issue.

Thanks to Scott Wood for providing the errata data which led to
a much cleaner fix.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 arch/powerpc/kernel/legacy_serial.c |   11 +++++++++++
 drivers/tty/serial/8250.c           |   11 ++++++++++-
 include/linux/serial_8250.h         |    5 +++++
 3 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index c7b5afe..dd232ca 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -476,6 +476,15 @@ static void __init fixup_port_mmio(int index,
 	port->membase = ioremap(port->mapbase, 0x100);
 }
 
+static void __init fixup_port_bugs(int index,
+				   struct device_node *np,
+				   struct plat_serial8250_port *port)
+{
+	DBG("fixup_port_bugs(%d)\n", index);
+
+	if (of_device_is_compatible(np, "fsl,ns16550"))
+		port->bugs = UART_BUG_FSLBK;
+}
 /*
  * This is called as an arch initcall, hopefully before the PCI bus is
  * probed and/or the 8250 driver loaded since we need to register our
@@ -512,6 +521,8 @@ static int __init serial_dev_init(void)
 			fixup_port_pio(i, np, port);
 		if ((port->iotype == UPIO_MEM) || (port->iotype == UPIO_TSI))
 			fixup_port_mmio(i, np, port);
+
+		fixup_port_bugs(i, np, port);
 	}
 
 	DBG("Registering platform serial ports\n");
diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index f99f27c..32e9821 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -142,6 +142,7 @@ struct uart_8250_port {
 	unsigned char		mcr_mask;	/* mask of user bits */
 	unsigned char		mcr_force;	/* mask of forced bits */
 	unsigned char		cur_iotype;	/* Running I/O type */
+	unsigned char		lsr_last;	/* LSR of last IRQ event */
 
 	/*
 	 * Some bits in registers are cleared on a read, so they must
@@ -1553,7 +1554,15 @@ static void serial8250_handle_port(struct uart_8250_port *up)
 
 	spin_lock_irqsave(&up->port.lock, flags);
 
-	status = serial_inp(up, UART_LSR);
+	/* Workaround for IRQ storm errata on break with Freescale 16550 */
+	if (UART_BUG_FSLBK & up->port.bugs && up->lsr_last & UART_LSR_BI) {
+		up->lsr_last &= ~UART_LSR_BI;
+		serial_inp(up, UART_RX);
+		spin_unlock_irqrestore(&up->port.lock, flags);
+		return;
+	}
+
+	status = up->lsr_last = serial_inp(up, UART_LSR);
 
 	DEBUG_INTR("status = %x...", status);
 
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 8c660af..b0f4042 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -18,6 +18,11 @@
 #define UART_BUG_TXEN	(1 << 1)	/* buggy TX IIR status */
 #define UART_BUG_NOMSR	(1 << 2)	/* buggy MSR status bits (Au1x00) */
 #define UART_BUG_THRE	(1 << 3)	/* buggy THRE reassertion */
+#ifdef CONFIG_PPC32
+#define UART_BUG_FSLBK	(1 << 4)	/* buggy FSL break IRQ storm */
+#else	/* help GCC optimize away IRQ handler errata code for ARCH != PPC32 */
+#define UART_BUG_FSLBK	0
+#endif
 
 /*
  * This is the platform device platform_data structure
-- 
1.7.7

Re: [PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Scott Wood]

On 12/01/2011 05:47 PM, Paul Gortmaker wrote:
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index 8c660af..b0f4042 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -18,6 +18,11 @@
>  #define UART_BUG_TXEN	(1 << 1)	/* buggy TX IIR status */
>  #define UART_BUG_NOMSR	(1 << 2)	/* buggy MSR status bits (Au1x00) */
>  #define UART_BUG_THRE	(1 << 3)	/* buggy THRE reassertion */
> +#ifdef CONFIG_PPC32
> +#define UART_BUG_FSLBK	(1 << 4)	/* buggy FSL break IRQ storm */
> +#else	/* help GCC optimize away IRQ handler errata code for ARCH != PPC32 */
> +#define UART_BUG_FSLBK	0
> +#endif

I believe this bug still exists on our 64-bit chips.

-Scott

Re: [PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Paul Gortmaker]

On 11-12-01 06:51 PM, Scott Wood wrote:
> On 12/01/2011 05:47 PM, Paul Gortmaker wrote:
>> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> index 8c660af..b0f4042 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -18,6 +18,11 @@
>>  #define UART_BUG_TXEN	(1 << 1)	/* buggy TX IIR status */
>>  #define UART_BUG_NOMSR	(1 << 2)	/* buggy MSR status bits (Au1x00) */
>>  #define UART_BUG_THRE	(1 << 3)	/* buggy THRE reassertion */
>> +#ifdef CONFIG_PPC32
>> +#define UART_BUG_FSLBK	(1 << 4)	/* buggy FSL break IRQ storm */
>> +#else	/* help GCC optimize away IRQ handler errata code for ARCH != PPC32 */
>> +#define UART_BUG_FSLBK	0
>> +#endif
> 
> I believe this bug still exists on our 64-bit chips.

OK, I'll simply change the above to CONFIG_PPC then.

Thanks,
Paul.

> 
> -Scott
> 

Re: [PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Kumar Gala]

On Dec 1, 2011, at 6:05 PM, Paul Gortmaker wrote:

> On 11-12-01 06:51 PM, Scott Wood wrote:
>> On 12/01/2011 05:47 PM, Paul Gortmaker wrote:
>>> diff --git a/include/linux/serial_8250.h =
b/include/linux/serial_8250.h
>>> index 8c660af..b0f4042 100644
>>> --- a/include/linux/serial_8250.h
>>> +++ b/include/linux/serial_8250.h
>>> @@ -18,6 +18,11 @@
>>> #define UART_BUG_TXEN	(1 << 1)	/* buggy TX IIR status =
*/
>>> #define UART_BUG_NOMSR	(1 << 2)	/* buggy MSR status bits =
(Au1x00) */
>>> #define UART_BUG_THRE	(1 << 3)	/* buggy THRE =
reassertion */
>>> +#ifdef CONFIG_PPC32
>>> +#define UART_BUG_FSLBK	(1 << 4)	/* buggy FSL break IRQ =
storm */
>>> +#else	/* help GCC optimize away IRQ handler errata code for =
ARCH !=3D PPC32 */
>>> +#define UART_BUG_FSLBK	0
>>> +#endif
>>=20
>> I believe this bug still exists on our 64-bit chips.
>=20
> OK, I'll simply change the above to CONFIG_PPC then.

It does, the bug is in the uart IP which I don't think we ever plan on =
fixing, so 32 or 64-bit parts will have it for ever and ever ;)

- k=

Re: [PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Alan Cox]

> > OK, I'll simply change the above to CONFIG_PPC then.
> 
> It does, the bug is in the uart IP which I don't think we ever plan on fixing, so 32 or 64-bit parts will have it for ever and ever ;)

It should be runtime selected, there should be no ifdefs here.

Re: [PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Paul Gortmaker]

On 11-12-02 06:30 AM, Alan Cox wrote:
>>> OK, I'll simply change the above to CONFIG_PPC then.
>>
>> It does, the bug is in the uart IP which I don't think we ever plan on fixing, so 32 or 64-bit parts will have it for ever and ever ;)
> 
> It should be runtime selected, there should be no ifdefs here.

The ifdef wasn't strictly required; it just made it so gcc would
toss the errata code out of the irq handler for !PPC.  Anyway it
will be a moot point if I can somehow hide all the mess by snooping
serial_inp() traffic and deploying the errata fix from there....

P.

> 

Re: [PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Scott Wood]

On 12/02/2011 10:34 AM, Paul Gortmaker wrote:
> On 11-12-02 06:30 AM, Alan Cox wrote:
>>>> OK, I'll simply change the above to CONFIG_PPC then.
>>>
>>> It does, the bug is in the uart IP which I don't think we ever plan on fixing, so 32 or 64-bit parts will have it for ever and ever ;)
>>
>> It should be runtime selected, there should be no ifdefs here.
> 
> The ifdef wasn't strictly required; it just made it so gcc would
> toss the errata code out of the irq handler for !PPC.  Anyway it
> will be a moot point if I can somehow hide all the mess by snooping
> serial_inp() traffic and deploying the errata fix from there....

Eww.

If it's not to be allowed in the main 8250 code (even for ppc builds
only), a custom handle_port sounds like a saner option.

-Scott

Re: [PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Alan Cox]

> @@ -1553,7 +1554,15 @@ static void serial8250_handle_port(struct
> uart_8250_port *up) 
>  	spin_lock_irqsave(&up->port.lock, flags);
>  
> -	status = serial_inp(up, UART_LSR);
> +	/* Workaround for IRQ storm errata on break with Freescale
> 16550 */
> +	if (UART_BUG_FSLBK & up->port.bugs && up->lsr_last &
> UART_LSR_BI) {
> +		up->lsr_last &= ~UART_LSR_BI;
> +		serial_inp(up, UART_RX);
> +		spin_unlock_irqrestore(&up->port.lock, flags);
> +		return;
> +	}
> +
> +	status = up->lsr_last = serial_inp(up, UART_LSR);

We've now had a recent pile of IRQ function changes adding more quirk
bits and special casing. This doesn't scale. We either need to make
handle_port a method the specific drivers can override or you could
hide the mess in your serial_inp implementation and not touch any core
code.

I really don't mind which but I suspect dealing with it in your
serial_inp handler so that when you read UART_LSR you do the fixup
might be simplest providing you can just do that.

Sorting out a ->handle_port override is probably ultimately the right
thing to do and then we can push some of the other funnies out further.

At this point it's becoming increasingly clear that 8250 UART cloners
are both very bad at cloning the things accurately, and very busy adding
extra useful features so we need to start to treat 8250.c as a library
you can wrap.

Alan

Re: [PATCH 3/3] 8250: add workaround for MPC8[356]xx UART break IRQ storm

[Paul Gortmaker]

On Thu, Dec 1, 2011 at 7:57 PM, Alan Cox <alan@linux.intel.com> wrote:
>
>> @@ -1553,7 +1554,15 @@ static void serial8250_handle_port(struct
>> uart_8250_port *up)
>> =A0 =A0 =A0 spin_lock_irqsave(&up->port.lock, flags);
>>
>> - =A0 =A0 status =3D serial_inp(up, UART_LSR);
>> + =A0 =A0 /* Workaround for IRQ storm errata on break with Freescale
>> 16550 */
>> + =A0 =A0 if (UART_BUG_FSLBK & up->port.bugs && up->lsr_last &
>> UART_LSR_BI) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 up->lsr_last &=3D ~UART_LSR_BI;
>> + =A0 =A0 =A0 =A0 =A0 =A0 serial_inp(up, UART_RX);
>> + =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock_irqrestore(&up->port.lock, flags);
>> + =A0 =A0 =A0 =A0 =A0 =A0 return;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 status =3D up->lsr_last =3D serial_inp(up, UART_LSR);
>
> We've now had a recent pile of IRQ function changes adding more quirk
> bits and special casing. This doesn't scale. We either need to make
> handle_port a method the specific drivers can override or you could
> hide the mess in your serial_inp implementation and not touch any core
> code.

To be fair, this one is zero cost for !PPC, but I understand your point,
and the idea of hiding it somehow in serial_inp was something that
never crossed my mind.  I'll look into seeing if I can abuse that.

>
> I really don't mind which but I suspect dealing with it in your
> serial_inp handler so that when you read UART_LSR you do the fixup
> might be simplest providing you can just do that.
>
> Sorting out a ->handle_port override is probably ultimately the right
> thing to do and then we can push some of the other funnies out further.
>
> At this point it's becoming increasingly clear that 8250 UART cloners
> are both very bad at cloning the things accurately, and very busy adding
> extra useful features so we need to start to treat 8250.c as a library
> you can wrap.

Sad but true.  It is like a time warp back to lib8390.c  --- whee.

P.

>
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" i=
n
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

[PATCH 0/6] RFCv2 Fix Fsl 8250 BRK bug

[Paul Gortmaker]

Alan wrote:

> Things like 8250_dw.c is perhaps more the style we want to be looking at
> for other devices and adjusting 8250.c as needed to export or split
> methods up so they can be used as helpers.

[...]

> Sorting out a ->handle_port override is probably ultimately the right
> thing to do and then we can push some of the other funnies out further.

I never figured spending this much time on this, but I think this is
a lot better, and 8250.c comes out slightly smaller and cleaner too,
hopefully satisfying Alan's other request "...we want less not more".

The errata is firewalled off in 8250_fsl.c (as per Alan's 1st comment),
and 8250.c now exports a couple more things to be used as helpers.

When I started looking at a handle_port override, I realized that it
really didn't need to exist at all, and the existing handle_irq
override we already have does the job nicely.  In the process, I found
an interesting quirk while cleaning up duplicated code in the timeout
handler, where we don't use the custom IRQ handler if there is one.

So, the errata is dealt with by installing a custom IRQ handler, and
it doesn't have the "eew ick" factor that Scott mentioned, in regards
to trying to learn state from watching serial_in() traffic.

The timeout cleanup seemed so obvious, that I was convinced I must
be missing some subtle thing.  So I confirmed it by stripping my IRQ
properties from my dts file and running the serial console IRQ-less.

I also re-tested the sysRq feature on sbc8349, to ensure the WAR was
actually doing its job, and I also tested with serial console on
an old SocketA board to make sure non-ppc didn't get hosed somehow.

Anyway, have a look and see if this version of things is acceptable
to all.  (Again, the dts update from Kumar isn't shown here).

Thanks to all who provided the feedback on v1.
Paul.

------

Paul Gortmaker (6):
  serial: move struct uart_8250_port from 8250.c to 8250.h
  serial: clean up parameter passing for 8250 Rx IRQ handling
  serial: export the key functions for an 8250 IRQ handler
  serial: make 8250 timeout use the specified IRQ handler
  serial: manually inline serial8250_handle_port
  serial: add irq handler for Freescale 16550 errata.

 arch/powerpc/kernel/legacy_serial.c |    3 +
 drivers/tty/serial/8250.c           |   92 ++++++++++++-----------------------
 drivers/tty/serial/8250.h           |   26 ++++++++++
 drivers/tty/serial/8250_fsl.c       |   63 ++++++++++++++++++++++++
 drivers/tty/serial/Kconfig          |    5 ++
 drivers/tty/serial/Makefile         |    1 +
 include/linux/serial_8250.h         |    5 ++
 7 files changed, 134 insertions(+), 61 deletions(-)
 create mode 100644 drivers/tty/serial/8250_fsl.c

-- 
1.7.7

[PATCH 1/6] serial: move struct uart_8250_port from 8250.c to 8250.h

[Paul Gortmaker]

Since we want to promote sharing and move away from one single
uart driver with a bunch of platform specific bugfixes all
munged into one, relocate some header like material from
the C file to the header.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/8250.c |   26 --------------------------
 drivers/tty/serial/8250.h |   26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index eeadf1b..d97628e 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -129,32 +129,6 @@ static unsigned long probe_rsa[PORT_RSA_MAX];
 static unsigned int probe_rsa_count;
 #endif /* CONFIG_SERIAL_8250_RSA  */
 
-struct uart_8250_port {
-	struct uart_port	port;
-	struct timer_list	timer;		/* "no irq" timer */
-	struct list_head	list;		/* ports on this IRQ */
-	unsigned short		capabilities;	/* port capabilities */
-	unsigned short		bugs;		/* port bugs */
-	unsigned int		tx_loadsz;	/* transmit fifo load size */
-	unsigned char		acr;
-	unsigned char		ier;
-	unsigned char		lcr;
-	unsigned char		mcr;
-	unsigned char		mcr_mask;	/* mask of user bits */
-	unsigned char		mcr_force;	/* mask of forced bits */
-	unsigned char		cur_iotype;	/* Running I/O type */
-
-	/*
-	 * Some bits in registers are cleared on a read, so they must
-	 * be saved whenever the register is read but the bits will not
-	 * be immediately processed.
-	 */
-#define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
-	unsigned char		lsr_saved_flags;
-#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
-	unsigned char		msr_saved_flags;
-};
-
 struct irq_info {
 	struct			hlist_node node;
 	int			irq;
diff --git a/drivers/tty/serial/8250.h b/drivers/tty/serial/8250.h
index 6edf4a6..ae027be 100644
--- a/drivers/tty/serial/8250.h
+++ b/drivers/tty/serial/8250.h
@@ -13,6 +13,32 @@
 
 #include <linux/serial_8250.h>
 
+struct uart_8250_port {
+	struct uart_port	port;
+	struct timer_list	timer;		/* "no irq" timer */
+	struct list_head	list;		/* ports on this IRQ */
+	unsigned short		capabilities;	/* port capabilities */
+	unsigned short		bugs;		/* port bugs */
+	unsigned int		tx_loadsz;	/* transmit fifo load size */
+	unsigned char		acr;
+	unsigned char		ier;
+	unsigned char		lcr;
+	unsigned char		mcr;
+	unsigned char		mcr_mask;	/* mask of user bits */
+	unsigned char		mcr_force;	/* mask of forced bits */
+	unsigned char		cur_iotype;	/* Running I/O type */
+
+	/*
+	 * Some bits in registers are cleared on a read, so they must
+	 * be saved whenever the register is read but the bits will not
+	 * be immediately processed.
+	 */
+#define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
+	unsigned char		lsr_saved_flags;
+#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
+	unsigned char		msr_saved_flags;
+};
+
 struct old_serial_port {
 	unsigned int uart;
 	unsigned int baud_base;
-- 
1.7.7

[PATCH 2/6] serial: clean up parameter passing for 8250 Rx IRQ handling

[Paul Gortmaker]

The receive_chars() was taking a pointer to a passed in LSR value
in status and knocking off bits as it processed them.  But since
receive_chars isn't returning a value, we can instead pass in
a normal non-pointer value for LSR, and simply return the
residual (unprocessed) LSR once it is done.

The value in this cleanup, is that it clarifies the API of the
receive_chars prior to exporting it to other 8250-like drivers
for shared usage.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/8250.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index d97628e..a20d7eb 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -1375,11 +1375,16 @@ static void clear_rx_fifo(struct uart_8250_port *up)
 	} while (1);
 }
 
-static void
-receive_chars(struct uart_8250_port *up, unsigned int *status)
+/*
+ * receive_chars: processes according to the passed in LSR
+ * value, and returns the remaining LSR bits not handled
+ * by this Rx routine.
+ */
+static unsigned char
+receive_chars(struct uart_8250_port *up, unsigned char lsr)
 {
 	struct tty_struct *tty = up->port.state->port.tty;
-	unsigned char ch, lsr = *status;
+	unsigned char ch;
 	int max_count = 256;
 	char flag;
 
@@ -1455,7 +1460,7 @@ ignore_char:
 	spin_unlock(&up->port.lock);
 	tty_flip_buffer_push(tty);
 	spin_lock(&up->port.lock);
-	*status = lsr;
+	return lsr;
 }
 
 static void transmit_chars(struct uart_8250_port *up)
@@ -1524,7 +1529,7 @@ static unsigned int check_modem_status(struct uart_8250_port *up)
  */
 static void serial8250_handle_port(struct uart_8250_port *up)
 {
-	unsigned int status;
+	unsigned char status;
 	unsigned long flags;
 
 	spin_lock_irqsave(&up->port.lock, flags);
@@ -1534,7 +1539,7 @@ static void serial8250_handle_port(struct uart_8250_port *up)
 	DEBUG_INTR("status = %x...", status);
 
 	if (status & (UART_LSR_DR | UART_LSR_BI))
-		receive_chars(up, &status);
+		status = receive_chars(up, status);
 	check_modem_status(up);
 	if (status & UART_LSR_THRE)
 		transmit_chars(up);
-- 
1.7.7

[PATCH 3/6] serial: export the key functions for an 8250 IRQ handler

[Paul Gortmaker]

For drivers that need to construct their own IRQ handler, the
three components are seen in the current handle_port -- i.e.
Rx, Tx and modem_status.

Make these exported symbols so that "almost" 8250 UARTs can
construct their own IRQ handler with these shared components,
while working around their own unique errata issues.

The function names are given a serial8250 prefix, since they
are now entering the global namespace.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/8250.c   |   29 +++++++++++++++--------------
 include/linux/serial_8250.h |    4 ++++
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index a20d7eb..91afe7a 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -1300,8 +1300,6 @@ static void serial8250_stop_tx(struct uart_port *port)
 	}
 }
 
-static void transmit_chars(struct uart_8250_port *up);
-
 static void serial8250_start_tx(struct uart_port *port)
 {
 	struct uart_8250_port *up =
@@ -1318,7 +1316,7 @@ static void serial8250_start_tx(struct uart_port *port)
 			if ((up->port.type == PORT_RM9000) ?
 				(lsr & UART_LSR_THRE) :
 				(lsr & UART_LSR_TEMT))
-				transmit_chars(up);
+				serial8250_tx_chars(up);
 		}
 	}
 
@@ -1376,12 +1374,12 @@ static void clear_rx_fifo(struct uart_8250_port *up)
 }
 
 /*
- * receive_chars: processes according to the passed in LSR
+ * serial8250_rx_chars: processes according to the passed in LSR
  * value, and returns the remaining LSR bits not handled
  * by this Rx routine.
  */
-static unsigned char
-receive_chars(struct uart_8250_port *up, unsigned char lsr)
+unsigned char
+serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
 {
 	struct tty_struct *tty = up->port.state->port.tty;
 	unsigned char ch;
@@ -1462,8 +1460,9 @@ ignore_char:
 	spin_lock(&up->port.lock);
 	return lsr;
 }
+EXPORT_SYMBOL_GPL(serial8250_rx_chars);
 
-static void transmit_chars(struct uart_8250_port *up)
+void serial8250_tx_chars(struct uart_8250_port *up)
 {
 	struct circ_buf *xmit = &up->port.state->xmit;
 	int count;
@@ -1500,8 +1499,9 @@ static void transmit_chars(struct uart_8250_port *up)
 	if (uart_circ_empty(xmit))
 		__stop_tx(up);
 }
+EXPORT_SYMBOL_GPL(serial8250_tx_chars);
 
-static unsigned int check_modem_status(struct uart_8250_port *up)
+unsigned int serial8250_modem_status(struct uart_8250_port *up)
 {
 	unsigned int status = serial_in(up, UART_MSR);
 
@@ -1523,6 +1523,7 @@ static unsigned int check_modem_status(struct uart_8250_port *up)
 
 	return status;
 }
+EXPORT_SYMBOL_GPL(serial8250_modem_status);
 
 /*
  * This handles the interrupt from one port.
@@ -1539,10 +1540,10 @@ static void serial8250_handle_port(struct uart_8250_port *up)
 	DEBUG_INTR("status = %x...", status);
 
 	if (status & (UART_LSR_DR | UART_LSR_BI))
-		status = receive_chars(up, status);
-	check_modem_status(up);
+		status = serial8250_rx_chars(up, status);
+	serial8250_modem_status(up);
 	if (status & UART_LSR_THRE)
-		transmit_chars(up);
+		serial8250_tx_chars(up);
 
 	spin_unlock_irqrestore(&up->port.lock, flags);
 }
@@ -1780,7 +1781,7 @@ static void serial8250_backup_timeout(unsigned long data)
 	}
 
 	if (!(iir & UART_IIR_NO_INT))
-		transmit_chars(up);
+		serial8250_tx_chars(up);
 
 	if (is_real_interrupt(up->port.irq))
 		serial_out(up, UART_IER, ier);
@@ -1814,7 +1815,7 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
 	unsigned int status;
 	unsigned int ret;
 
-	status = check_modem_status(up);
+	status = serial8250_modem_status(up);
 
 	ret = 0;
 	if (status & UART_MSR_DCD)
@@ -2861,7 +2862,7 @@ serial8250_console_write(struct console *co, const char *s, unsigned int count)
 	 *	while processing with interrupts off.
 	 */
 	if (up->msr_saved_flags)
-		check_modem_status(up);
+		serial8250_modem_status(up);
 
 	if (locked)
 		spin_unlock(&up->port.lock);
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 1f05bbe..b44034e 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -66,6 +66,7 @@ enum {
  * dependent on the 8250 driver.
  */
 struct uart_port;
+struct uart_8250_port;
 
 int serial8250_register_port(struct uart_port *);
 void serial8250_unregister_port(int line);
@@ -82,6 +83,9 @@ extern void serial8250_do_set_termios(struct uart_port *port,
 extern void serial8250_do_pm(struct uart_port *port, unsigned int state,
 			     unsigned int oldstate);
 int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
+unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
+void serial8250_tx_chars(struct uart_8250_port *up);
+unsigned int serial8250_modem_status(struct uart_8250_port *up);
 
 extern void serial8250_set_isa_configurator(void (*v)
 					(int port, struct uart_port *up,
-- 
1.7.7

[PATCH 4/6] serial: make 8250 timeout use the specified IRQ handler

[Paul Gortmaker]

The current 8250 timeout code duplicates the code path in
serial8250_default_handle_irq and then serial8250_handle_irq
i.e. reading iir, check for IIR_NO_INT, and then calling
serial8250_handle_port.

So the immediate thought is to replace the duplicated code
with a call to serial8250_default_handle_irq.

But this highlights a problem.  We let 8250 driver variants
use their own IRQ handler via specifying their own custom
->handle_irq, but in the event of a timeout, we ignore their
handler and implicitly run serial8250_default_handle_irq instead.

So, go through the struct to get ->handle_irq and call that,
which for most will still be serial8250_default_handle_irq.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/8250.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 91afe7a..9e7780d 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -1738,11 +1738,8 @@ static void serial_unlink_irq_chain(struct uart_8250_port *up)
 static void serial8250_timeout(unsigned long data)
 {
 	struct uart_8250_port *up = (struct uart_8250_port *)data;
-	unsigned int iir;
 
-	iir = serial_in(up, UART_IIR);
-	if (!(iir & UART_IIR_NO_INT))
-		serial8250_handle_port(up);
+	up->port.handle_irq(&up->port);
 	mod_timer(&up->timer, jiffies + uart_poll_timeout(&up->port));
 }
 
-- 
1.7.7

[PATCH 5/6] serial: manually inline serial8250_handle_port

[Paul Gortmaker]

Currently serial8250_handle_irq is a trivial wrapper around
serial8250_handle_port, which actually does all the work.

Since there are no other callers of serial8250_handle_port, we
can just move it inline into serial8250_handle_irq.  This also
makes it more clear what functionality any custom IRQ handlers
need to provide if not using serial8250_default_handle_irq.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/8250.c |   23 ++++++++---------------
 1 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/8250.c b/drivers/tty/serial/8250.c
index 9e7780d..23332cb 100644
--- a/drivers/tty/serial/8250.c
+++ b/drivers/tty/serial/8250.c
@@ -1528,10 +1528,15 @@ EXPORT_SYMBOL_GPL(serial8250_modem_status);
 /*
  * This handles the interrupt from one port.
  */
-static void serial8250_handle_port(struct uart_8250_port *up)
+int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 {
 	unsigned char status;
 	unsigned long flags;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+
+	if (iir & UART_IIR_NO_INT)
+		return 0;
 
 	spin_lock_irqsave(&up->port.lock, flags);
 
@@ -1546,19 +1551,7 @@ static void serial8250_handle_port(struct uart_8250_port *up)
 		serial8250_tx_chars(up);
 
 	spin_unlock_irqrestore(&up->port.lock, flags);
-}
-
-int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
-{
-	struct uart_8250_port *up =
-		container_of(port, struct uart_8250_port, port);
-
-	if (!(iir & UART_IIR_NO_INT)) {
-		serial8250_handle_port(up);
-		return 1;
-	}
-
-	return 0;
+	return 1;
 }
 EXPORT_SYMBOL_GPL(serial8250_handle_irq);
 
@@ -2825,7 +2818,7 @@ serial8250_console_write(struct console *co, const char *s, unsigned int count)
 
 	local_irq_save(flags);
 	if (up->port.sysrq) {
-		/* serial8250_handle_port() already took the lock */
+		/* serial8250_handle_irq() already took the lock */
 		locked = 0;
 	} else if (oops_in_progress) {
 		locked = spin_trylock(&up->port.lock);
-- 
1.7.7

[PATCH 6/6] serial: add irq handler for Freescale 16550 errata.

[Paul Gortmaker]

Sending a break on the SOC UARTs found in some MPC83xx/85xx/86xx
chips seems to cause a short lived IRQ storm (/proc/interrupts
typically shows somewhere between 300 and 1500 events).  Unfortunately
this renders SysRQ over the serial console completely inoperable.

The suggested workaround in the errata is to read the Rx register,
wait one character period, and then read the Rx register again.
We achieve this by tracking the old LSR value, and on the subsequent
interrupt event after a break, we don't read LSR, instead we just
read the RBR again and return immediately.

The "fsl,ns16550" is used in the compatible field of the serial
device to mark UARTs known to have this issue.

Thanks to Scott Wood for providing the errata data which led to
a much cleaner fix.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 arch/powerpc/kernel/legacy_serial.c |    3 ++
 drivers/tty/serial/8250_fsl.c       |   63 +++++++++++++++++++++++++++++++++++
 drivers/tty/serial/Kconfig          |    5 +++
 drivers/tty/serial/Makefile         |    1 +
 include/linux/serial_8250.h         |    1 +
 5 files changed, 73 insertions(+), 0 deletions(-)
 create mode 100644 drivers/tty/serial/8250_fsl.c

diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index c7b5afe..3fea368 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -441,6 +441,9 @@ static void __init fixup_port_irq(int index,
 		return;
 
 	port->irq = virq;
+
+	if (of_device_is_compatible(np, "fsl,ns16550"))
+		port->handle_irq = fsl8250_handle_irq;
 }
 
 static void __init fixup_port_pio(int index,
diff --git a/drivers/tty/serial/8250_fsl.c b/drivers/tty/serial/8250_fsl.c
new file mode 100644
index 0000000..f4d3c47
--- /dev/null
+++ b/drivers/tty/serial/8250_fsl.c
@@ -0,0 +1,63 @@
+#include <linux/serial_reg.h>
+#include <linux/serial_8250.h>
+
+#include "8250.h"
+
+/*
+ * Freescale 16550 UART "driver", Copyright (C) 2011 Paul Gortmaker.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This isn't a full driver; it just provides an alternate IRQ
+ * handler to deal with an errata.  Everything else is just
+ * using the bog standard 8250 support.
+ *
+ * We follow code flow of serial8250_default_handle_irq() but add
+ * a check for a break and insert a dummy read on the Rx for the
+ * immediately following IRQ event.
+ *
+ * We re-use the already existing "bug handling" lsr_saved_flags
+ * field to carry the "what we just did" information from the one
+ * IRQ event to the next one.
+ */
+
+int fsl8250_handle_irq(struct uart_port *port)
+{
+	unsigned char lsr, orig_lsr;
+	unsigned long flags;
+	unsigned int iir;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+
+	spin_lock_irqsave(&up->port.lock, flags);
+
+	iir = port->serial_in(port, UART_IIR);
+	if (iir & UART_IIR_NO_INT) {
+		spin_unlock_irqrestore(&up->port.lock, flags);
+		return 0;
+	}
+
+	/* This is the WAR; if last event was BRK, then read and return */
+	if (unlikely(up->lsr_saved_flags & UART_LSR_BI)) {
+		up->lsr_saved_flags &= ~UART_LSR_BI;
+		port->serial_in(port, UART_RX);
+		spin_unlock_irqrestore(&up->port.lock, flags);
+		return 1;
+	}
+
+	lsr = orig_lsr = up->port.serial_in(&up->port, UART_LSR);
+
+	if (lsr & (UART_LSR_DR | UART_LSR_BI))
+		lsr = serial8250_rx_chars(up, lsr);
+
+	serial8250_modem_status(up);
+
+	if (lsr & UART_LSR_THRE)
+		serial8250_tx_chars(up);
+
+	up->lsr_saved_flags = orig_lsr;
+	spin_unlock_irqrestore(&up->port.lock, flags);
+	return 1;
+}
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 925a1e5..aa46993 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -97,6 +97,11 @@ config SERIAL_8250_PNP
 	  This builds standard PNP serial support. You may be able to
 	  disable this feature if you only need legacy serial support.
 
+config SERIAL_8250_FSL
+	bool
+	depends on SERIAL_8250 && PPC
+	default PPC
+
 config SERIAL_8250_HP300
 	tristate
 	depends on SERIAL_8250 && HP300
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index e10cf5b..fb187a3 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_BOCA) += 8250_boca.o
 obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554) += 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6) += 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_MCA) += 8250_mca.o
+obj-$(CONFIG_SERIAL_8250_FSL) += 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW) += 8250_dw.o
 obj-$(CONFIG_SERIAL_AMBA_PL010) += amba-pl010.o
 obj-$(CONFIG_SERIAL_AMBA_PL011) += amba-pl011.o
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index b44034e..8f012f8 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -82,6 +82,7 @@ extern void serial8250_do_set_termios(struct uart_port *port,
 		struct ktermios *termios, struct ktermios *old);
 extern void serial8250_do_pm(struct uart_port *port, unsigned int state,
 			     unsigned int oldstate);
+extern int fsl8250_handle_irq(struct uart_port *port);
 int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
 unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
 void serial8250_tx_chars(struct uart_8250_port *up);
-- 
1.7.7

Re: [PATCH 0/6] RFCv2 Fix Fsl 8250 BRK bug

[Alan Cox]

> Anyway, have a look and see if this version of things is acceptable
> to all.  (Again, the dts update from Kumar isn't shown here).
> 
> Thanks to all who provided the feedback on v1.

Looks good to me

Acked-by: Alan Cox <alan@linux.intel.com>