| Message ID | 490F51E7.3020309@unicontrol.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Grant Likely |
| Headers | show |
On Mon, Nov 3, 2008 at 12:32 PM, René Bürgel <r.buergel@unicontrol.de> wrote: > Hi > > This patch is a workaround for bug #364 found in the MPC52xx processor. > The errata document can be found under > http://www.freescale.com/files/32bit/doc/errata/MPC5200E.pdf?fpsp=1&WT_TYPE=Errata&WT_VENDOR=FREESCALE&WT_FILE_FORMAT=pdf&WT_ASSET=Documentation > This is an MPC5200 errata. It doesn't exist on the MPC5200B. The bugfix code should be enabled at runtime only if running on the original MPC5200. You can use the value of the compatible property to determine whether or not to enable it. Optionally you can further reduce impact by checking if CONFIG_PPC_MPC5200_BUGFIX is defined. More comments below > diff --git a/drivers/serial/mpc52xx_uart.c b/drivers/serial/mpc52xx_uart.c > index 6117d3d..929524b 100644 > --- a/drivers/serial/mpc52xx_uart.c > +++ b/drivers/serial/mpc52xx_uart.c > @@ -496,6 +496,27 @@ mpc52xx_uart_break_ctl(struct uart_port *port, int ctl) > spin_unlock_irqrestore(&port->lock, flags); > } > > +/* macro with helper macros to safely reset rx which mustn't be done in > break state. > + * This is a workaround for processor bug #364 described in MPC5200 (L25R) > Errata. > + * > + * The workaround resets the baudrate to 4800, waits for a stable state and > resets break state repeatedly if necessary > + * optionally it can release the lock while waiting. > + * 1 character at 4800 baud takes 2ms, 3ms should be enough for 1 character > at higher speed and 1 char at lowest > + * works only with longer delays > + */ > +#define LOCK(code) code > +#define DONT_LOCK(code) > +#define mpc52xx_uart_reset_rx(LOCK) > \ > + out_8(&psc->ctur,0x01); > \ > + out_8(&psc->ctlr,0xae); > \ > + do { > \ > + out_8(&psc->command,MPC52xx_PSC_RST_ERR_STAT); > \ > + LOCK(disable_irq(port->irq); > spin_unlock_irqrestore(&port->lock, flags)); \ > + mdelay(10); > \ > + LOCK(spin_lock_irqsave(&port->lock, flags); > enable_irq(port->irq)); \ > + } while ((in_be16(&psc->mpc52xx_psc_status)) & MPC52xx_PSC_SR_RB); > \ > + out_8(&psc->command,MPC52xx_PSC_RST_RX); > + ick. ugly. Don't mess about with a macro here, just write a function. Let the compiler decide if it should be inlined. g.
Grant Likely wrote: > On Mon, Nov 3, 2008 at 12:32 PM, René Bürgel <r.buergel@unicontrol.de> wrote: >> Hi >> >> This patch is a workaround for bug #364 found in the MPC52xx processor. >> The errata document can be found under >> http://www.freescale.com/files/32bit/doc/errata/MPC5200E.pdf?fpsp=1&WT_TYPE=Errata&WT_VENDOR=FREESCALE&WT_FILE_FORMAT=pdf&WT_ASSET=Documentation >> > > This is an MPC5200 errata. It doesn't exist on the MPC5200B. The > bugfix code should be enabled at runtime only if running on the > original MPC5200. You can use the value of the compatible property to > determine whether or not to enable it. I would hope not since the Efika uses mpc5200-psc and mpc5200-serial as it's compatible properties (this is from before mpc5200-psc-uart was defined), which would enable this bugfix across the board. Until we have a decent update for the device tree here (it's starting to cause some real trouble lately when people update stuff like this and want to compare) the best way to check for the MPC5200 or MPC5200B is to look at the PVR - you don't need the device tree for this, at all. Sometime this week I am going to go through the 5200b device tree in the kernel source and make sure efika.forth follows it as best as I can, and then make a release.. that won't fix anything for people who don't run the script, however. > Optionally you can further > reduce impact by checking if CONFIG_PPC_MPC5200_BUGFIX is defined. I would much prefer this.
On Mon, Nov 03, 2008 at 03:57:09PM -0600, Matt Sealey wrote: > > Optionally you can further >> reduce impact by checking if CONFIG_PPC_MPC5200_BUGFIX is defined. > > I would much prefer this. I submitted a patch to enable pipelining on a MPC5200B recently. It was disabled because of a bug in the MPC5200. The first version of this patch used MPC5200_BUGFIX and it was mentioned, that some people might want to run the same kernel on both kind of processors. So, the patch that went mainline checks for the PVR. Maybe we should stick to this here, too? All the best, Wolfram Sang
On Mon, Nov 3, 2008 at 3:15 PM, Wolfram Sang <w.sang@pengutronix.de> wrote: > On Mon, Nov 03, 2008 at 03:57:09PM -0600, Matt Sealey wrote: > >> > Optionally you can further >>> reduce impact by checking if CONFIG_PPC_MPC5200_BUGFIX is defined. >> >> I would much prefer this. > > I submitted a patch to enable pipelining on a MPC5200B recently. It was > disabled because of a bug in the MPC5200. The first version of this > patch used MPC5200_BUGFIX and it was mentioned, that some people might > want to run the same kernel on both kind of processors. So, the patch > that went mainline checks for the PVR. Maybe we should stick to this > here, too? Of the two solutions: 1. Run-time selection must be done. 2. Compile-time removal of the bug fix path can also be done to lessen runtime impact for kernels never run on older chips My view is that #1 is non-negotiable. #2 is a nice to have, but if it doesn't incur any cost when disabled at runtime then I don't care. g.
Wolfram Sang wrote: > On Mon, Nov 03, 2008 at 03:57:09PM -0600, Matt Sealey wrote: > >>> Optionally you can further >>> reduce impact by checking if CONFIG_PPC_MPC5200_BUGFIX is defined. >> I would much prefer this. > > I submitted a patch to enable pipelining on a MPC5200B recently. It was > disabled because of a bug in the MPC5200. The first version of this > patch used MPC5200_BUGFIX and it was mentioned, that some people might > want to run the same kernel on both kind of processors. So, the patch > that went mainline checks for the PVR. Maybe we should stick to this > here, too? I would wrap a real chipset errata inside CONFIG_PPC_MPC5200_BUGFIX so that you have the option to remove all code that fixes these errata, but also check the PVR and only do the bugfix if you're on that processor, so.. both :D The pipelining thing seems to be fixed in the MPC5200B but it actually makes the bus lock up under certain circumstances with the ATA DMA task and certain priority selections. I am not sure what we're going to do about that pipelining support (Tim Yamin's patch removed it! Maybe we should have a little addition to the BestComm driver which can set these things from the driver side using a little global API, so that if an ATA driver loads and wants this configuration, it can get to it)
diff --git a/drivers/serial/mpc52xx_uart.c b/drivers/serial/mpc52xx_uart.c index 6117d3d..929524b 100644 --- a/drivers/serial/mpc52xx_uart.c +++ b/drivers/serial/mpc52xx_uart.c @@ -496,6 +496,27 @@ mpc52xx_uart_break_ctl(struct uart_port *port, int ctl) spin_unlock_irqrestore(&port->lock, flags); } +/* macro with helper macros to safely reset rx which mustn't be done in break state. + * This is a workaround for processor bug #364 described in MPC5200 (L25R) Errata. + * + * The workaround resets the baudrate to 4800, waits for a stable state and resets break state repeatedly if necessary + * optionally it can release the lock while waiting. + * 1 character at 4800 baud takes 2ms, 3ms should be enough for 1 character at higher speed and 1 char at lowest + * works only with longer delays + */ +#define LOCK(code) code +#define DONT_LOCK(code) +#define mpc52xx_uart_reset_rx(LOCK) \ + out_8(&psc->ctur,0x01); \ + out_8(&psc->ctlr,0xae); \ + do { \ + out_8(&psc->command,MPC52xx_PSC_RST_ERR_STAT); \ + LOCK(disable_irq(port->irq); spin_unlock_irqrestore(&port->lock, flags)); \ + mdelay(10); \ + LOCK(spin_lock_irqsave(&port->lock, flags); enable_irq(port->irq)); \ + } while ((in_be16(&psc->mpc52xx_psc_status)) & MPC52xx_PSC_SR_RB); \ + out_8(&psc->command,MPC52xx_PSC_RST_RX); + static int mpc52xx_uart_startup(struct uart_port *port) { @@ -510,7 +531,7 @@ mpc52xx_uart_startup(struct uart_port *port) return ret; /* Reset/activate the port, clear and enable interrupts */ - out_8(&psc->command, MPC52xx_PSC_RST_RX); + mpc52xx_uart_reset_rx(DONT_LOCK); out_8(&psc->command, MPC52xx_PSC_RST_TX); out_be32(&psc->sicr, 0); /* UART mode DCD ignored */ @@ -529,7 +550,7 @@ mpc52xx_uart_shutdown(struct uart_port *port) struct mpc52xx_psc __iomem *psc = PSC(port); /* Shut down the port. Leave TX active if on a console port */ - out_8(&psc->command, MPC52xx_PSC_RST_RX); + mpc52xx_uart_reset_rx(DONT_LOCK); if (!uart_console(port)) out_8(&psc->command, MPC52xx_PSC_RST_TX); @@ -588,9 +609,6 @@ mpc52xx_uart_set_termios(struct uart_port *port, struct ktermios *new, /* Get the lock */ spin_lock_irqsave(&port->lock, flags); - /* Update the per-port timeout */ - uart_update_timeout(port, new->c_cflag, baud); - /* Do our best to flush TX & RX, so we don't loose anything */ /* But we don't wait indefinitly ! */ j = 5000000; /* Maximum wait */ @@ -607,9 +625,12 @@ mpc52xx_uart_set_termios(struct uart_port *port, struct ktermios *new, "Some chars may have been lost.\n"); /* Reset the TX & RX */ - out_8(&psc->command, MPC52xx_PSC_RST_RX); + mpc52xx_uart_reset_rx(LOCK); out_8(&psc->command, MPC52xx_PSC_RST_TX); + /* Update the per-port timeout */ + uart_update_timeout(port, new->c_cflag, baud); + /* Send new mode settings */ out_8(&psc->command, MPC52xx_PSC_SEL_MODE_REG_1); out_8(&psc->mode, mr1);