| Message ID | 1322783258-20443-4-git-send-email-paul.gortmaker@windriver.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
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
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 >
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 != 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. 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
> @@ -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
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) >> 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. 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" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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.
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. >
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
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
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(-)