[PATCH] workaround for mpc52xx erratum #364 (serial may not be reset in break state)
From: René Bürgel <hidden>
Date: 2008-11-03 19:32:14
16 messages, 4 authors, 2008-11-14 · open the first message on its own page
From: René Bürgel <hidden>
Date: 2008-11-03 19:32:14
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 When a device with a low baudrate is connected to the serial port, but the processor "listens" on a higher baudrate, it might falsely receive breaks from the controller. During a break, the serial controller may not be reset. The appended patch provides a workaround for that situation by lowering the baudrate without resetting the controller and waiting until no break is received anymore. -- René Bürgel Software Engineer Unicontrol Systemtechnik GmbH OT Dittersbach Sachsenburger Weg 34 09669 Frankenberg Tel.: 03 72 06/ 88 73 - 19 Fax: 03 72 06/ 88 73 - 60 E-Mail: r.buergel@unicontrol.de Internet: www.unicontrol.de Unicontrol Systemtechnik GmbH Geschäftsführer: Dipl.-Ing. Siegfried Heinze Sitz der Gesellschaft: Frankenberg Registergericht: Amtsgericht Chemnitz, HRB 15 475
From: Grant Likely <hidden>
Date: 2008-11-03 20:55:11
On Mon, Nov 3, 2008 at 12:32 PM, Ren=E9 B=FCrgel [off-list ref] = 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=3D1&WT_=
TYPE=3DErrata&WT_VENDOR=3DFREESCALE&WT_FILE_FORMAT=3Dpdf&WT_ASSET=3DDocumen= tation
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 c=
tl)
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 (L25=
R)
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 charac=
ter
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. --=20 Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.
From: Matt Sealey <hidden>
Date: 2008-11-03 21:57:19
Grant Likely wrote:
On Mon, Nov 3, 2008 at 12:32 PM, René Bürgel [off-list ref] wrote:quoted
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=DocumentationThis 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. -- Matt Sealey [off-list ref] Genesi, Manager, Developer Relations
From: Wolfram Sang <hidden>
Date: 2008-11-03 22:15:59
On Mon, Nov 03, 2008 at 03:57:09PM -0600, Matt Sealey wrote:
quoted
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 -- Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de Pengutronix - Linux Solutions for Science and Industry
From: Grant Likely <hidden>
Date: 2008-11-03 22:55:20
On Mon, Nov 3, 2008 at 3:15 PM, Wolfram Sang [off-list ref] wrote:
On Mon, Nov 03, 2008 at 03:57:09PM -0600, Matt Sealey wrote:quoted
quoted
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. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.
From: René Bürgel <hidden>
Date: 2008-11-04 10:44:15
Grant Likely schrieb:
On Mon, Nov 3, 2008 at 12:32 PM, René Bürgel [off-list ref] wrote:quoted
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=DocumentationThis 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.
This bug is definetly present on the MPC5200B, although it is not listed in the errata sheet. I've had the ability to test it. We have custom boards using CPU modules from TQ with both processor versions, MPC5200 and MPC5200B. Both versions show exactly the same behaviour and the patch fixes it. But as the serial driver is also used for the MPC5121, we may have to distinguish anyway. Does anyone have the possibility to test if the bug in still present on MPC5121? I disabled the bugfix on MPC5121-processors.
quoted
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 inbreak 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); \
The purpose of the macro wasn't to enforce inlining, but to enhance the readability of the call and to ensure, that the was no runtime-check needed to decide whether the lock has to be hold or may be released. I'd like to avoid the runtime-checking here Anyway, i transformed the macro into a function like suggested. Any proposals on this are welcome :) -- René Bürgel Software Engineer Unicontrol Systemtechnik GmbH OT Dittersbach Sachsenburger Weg 34 09669 Frankenberg Tel.: 03 72 06/ 88 73 - 19 Fax: 03 72 06/ 88 73 - 60 E-Mail: r.buergel@unicontrol.de Internet: www.unicontrol.de Unicontrol Systemtechnik GmbH Geschäftsführer: Dipl.-Ing. Siegfried Heinze Sitz der Gesellschaft: Frankenberg Registergericht: Amtsgericht Chemnitz, HRB 15 475
From: Wolfram Sang <hidden>
Date: 2008-11-04 11:15:32
Hello Rene, I haven't actually applied the patch, just a few comments from a glimpse:
+/* 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.
Minor nit: Kernel CodingStyle suggests long comments like this /* * comment starts here * ... */ Major nit: Please add to the comment that this bug is still present on the MPC5200B, although it is not in its errata sheet. Thils will avoid later confusion. (Out of interest, did you contact Freescale about this bug?)
+ * + * 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
Did I get it right that there are cases where the workaround won't work?
+ */ + +static void mpc52xx_uart_reset_rx(struct uart_port *port, int unlock, unsigned long flags)
'bool unlock'?
+{ + struct mpc52xx_psc __iomem *psc = PSC(port); +#ifdef CONFIG_PPC_MPC52xx + out_8(&psc->ctur,0x01); + out_8(&psc->ctlr,0xae);
Those are magic values. If you can't build them using defined constants, at least document them, please.
+ do { + out_8(&psc->command,MPC52xx_PSC_RST_ERR_STAT); + if (unlock) { + disable_irq(port->irq); + spin_unlock_irqrestore(&port->lock, flags); + } + mdelay(10); + if (unlock) { + spin_lock_irqsave(&port->lock, flags); + enable_irq(port->irq); + } + } while ((in_be16(&psc->mpc52xx_psc_status)) & MPC52xx_PSC_SR_RB); +#endif + out_8(&psc->command,MPC52xx_PSC_RST_RX); +} + static int mpc52xx_uart_startup(struct uart_port *port) {@@ -510,7 +541,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(port, false, 0); out_8(&psc->command, MPC52xx_PSC_RST_TX); out_be32(&psc->sicr, 0); /* UART mode DCD ignored */@@ -529,7 +560,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(port, false, 0); if (!uart_console(port)) out_8(&psc->command, MPC52xx_PSC_RST_TX);@@ -588,9 +619,6 @@ mpc52xx_uart_set_termios(struct uart_port *port, struct ktermios *new, /* Get the lock */ spin_lock_irqsave(&port->lock, flags);
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Thanks! Wolfram Sang -- Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de Pengutronix - Linux Solutions for Science and Industry
From: René Bürgel <hidden>
Date: 2008-11-04 14:14:36
Wolfram Sang schrieb:
Hello Rene, I haven't actually applied the patch, just a few comments from a glimpse: Major nit: Please add to the comment that this bug is still present on the MPC5200B, although it is not in its errata sheet. Thils will avoid later confusion. (Out of interest, did you contact Freescale about this bug?)
Comment added, contacting Freescale is still on my to-do-list :)
quoted
+ * + * 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 delaysDid I get it right that there are cases where the workaround won't work?
Yes, currently it doesn't work when receiving with less than 4800 baud. I'll try to fix this.
quoted
+{ + struct mpc52xx_psc __iomem *psc = PSC(port); +#ifdef CONFIG_PPC_MPC52xx + out_8(&psc->ctur,0x01); + out_8(&psc->ctlr,0xae);Those are magic values. If you can't build them using defined constants, at least document them, please.
The magic numbers will hopefully vanish after generalizing the patch to work with every baudrate.
Thanks! Wolfram Sang
Thanks for your suggestions -- René Bürgel Software Engineer Unicontrol Systemtechnik GmbH OT Dittersbach Sachsenburger Weg 34 09669 Frankenberg Tel.: 03 72 06/ 88 73 - 19 Fax: 03 72 06/ 88 73 - 60 E-Mail: r.buergel@unicontrol.de Internet: www.unicontrol.de Unicontrol Systemtechnik GmbH Geschäftsführer: Dipl.-Ing. Siegfried Heinze Sitz der Gesellschaft: Frankenberg Registergericht: Amtsgericht Chemnitz, HRB 15 475
From: Matt Sealey <hidden>
Date: 2008-11-04 18:18:41
Wolfram Sang wrote:
On Mon, Nov 03, 2008 at 03:57:09PM -0600, Matt Sealey wrote:quoted
quoted
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) -- Matt Sealey [off-list ref] Genesi, Manager, Developer Relations
From: Matt Sealey <hidden>
Date: 2008-11-04 18:23:37
René Bürgel wrote:
But as the serial driver is also used for the MPC5121, we may have to distinguish anyway. Does anyone have the possibility to test if the bug in still present on MPC5121?
Tell us what to do to get it to occur and what we're looking for and we have a bunch of boards at Genesi, someone will find the time to test it (if not them, then me :) Right now I'm having a hell of a time getting a 2.6.27.x kernel running on it though, some driver support is still missing from mainline making it just that little bit extra frustrating to work with.. (if you're using the system over a serial console, how are you supposed to test serial port operation? USB doesn't work in >2.6.24 so I guess I have to hope netconsole works :) -- Matt Sealey [off-list ref] Genesi, Manager, Developer Relations
From: René Bürgel <hidden>
Date: 2008-11-04 19:40:19
Hey, folks This is v3 of my patch to work around erratum #364 of the MPC5200(B). I removed most "magic" looking numbers, documenting the rest. As mentioned before, the previous patches weren't working for low baudrates (<9600). This should be fixed now. But there's still one thing, that bothers me a bit - if there is REALLY a break on the line, closing the driver may take until it's gone. I don't know whether this is really satisfying, but i think it's better than the alternative: no serial connection until the next reboot. -- René Bürgel Software Engineer Unicontrol Systemtechnik GmbH OT Dittersbach Sachsenburger Weg 34 09669 Frankenberg Tel.: 03 72 06/ 88 73 - 19 Fax: 03 72 06/ 88 73 - 60 E-Mail: r.buergel@unicontrol.de Internet: www.unicontrol.de Unicontrol Systemtechnik GmbH Geschäftsführer: Dipl.-Ing. Siegfried Heinze Sitz der Gesellschaft: Frankenberg Registergericht: Amtsgericht Chemnitz, HRB 15 475
From: Wolfram Sang <hidden>
Date: 2008-11-04 21:21:58
Hi René, On Tue, Nov 04, 2008 at 08:40:09PM +0100, René Bürgel wrote:
Hey, folks This is v3 of my patch to work around erratum #364 of the MPC5200(B). I removed most "magic" looking numbers, documenting the rest. As mentioned before, the previous patches weren't working for low baudrates (<9600). This should be fixed now. But there's still one thing, that bothers me a bit - if there is REALLY a break on the line, closing the driver may take until it's gone. I don't know whether this is really satisfying, but i think it's better than the alternative: no serial connection until the next reboot.
I think we should CC linux-serial to get some opinions about this. At least, if it stays like this, it should be mentioned in the source.
-- René Bürgel Software Engineer Unicontrol Systemtechnik GmbH OT Dittersbach Sachsenburger Weg 34 09669 Frankenberg Tel.: 03 72 06/ 88 73 - 19 Fax: 03 72 06/ 88 73 - 60 E-Mail: r.buergel@unicontrol.de Internet: www.unicontrol.de Unicontrol Systemtechnik GmbH Geschäftsführer: Dipl.-Ing. Siegfried Heinze Sitz der Gesellschaft: Frankenberg Registergericht: Amtsgericht Chemnitz, HRB 15 475
diff --git a/drivers/serial/mpc52xx_uart.c b/drivers/serial/mpc52xx_uart.c index 6117d3d..a2bb4d9 100644 --- a/drivers/serial/mpc52xx_uart.c +++ b/drivers/serial/mpc52xx_uart.c@@ -496,6 +496,59 @@ mpc52xx_uart_break_ctl(struct uart_port *port, int ctl) spin_unlock_irqrestore(&port->lock, flags); } +/* + * This is a workaround for processor bug #364 described in MPC5200 (L25R) Errata. + * The bug is still present in MPC5200B, but currently not listed in its errata sheet + * + * The workaround resets the baudrate to the slowest possible, + * waits for a stable state and resets break state repeatedly if necessary. + * Optionally it can release the lock while waiting. + * + * That baudrate is roughly port->uartclk / (1000 * 1000) + * The minimum wait time for the first try has to include the time to wait for stop-bits and a character, + * we wait for 2 chars to be sure + * Consecutive waits must just receive one character. + */
Here are lines >80 chars. Please let scripts/checkpatch.pl check this patch (--strict doesn't hurt). And please try also Documentation/CodingStyle.txt. It really helps maintaining that huge amount of code the kernel is.
+ +static void mpc52xx_uart_reset_rx(struct uart_port *port, bool unlock, unsigned long flags) +{ +#ifdef CONFIG_PPC_MPC52xx + void reset_errors_and_wait(struct uart_port *port, bool unlock, unsigned long flags, unsigned int delay) + { + struct mpc52xx_psc __iomem *psc = PSC(port); + out_8(&psc->command,MPC52xx_PSC_RST_ERR_STAT); + if (unlock) { + disable_irq(port->irq); + spin_unlock_irqrestore(&port->lock, flags); + } + mdelay( delay ); + if (unlock) { + spin_lock_irqsave(&port->lock, flags); + enable_irq(port->irq); + } + }
Function inside a function? Haven't seen this before in kernel code. I don't think it will be accepted, but leave this to maintainers.
+ + + struct mpc52xx_psc __iomem *psc = PSC(port); + + // One character on the serial port may consist of up to 12 bits. + // So the time to receive one char is 12 /*bits*/ / (port->uartclk / (1000 * 1000) /*MHz -> Hz*/) * 1000 /*s -> ms*/, + // but we'll wait 50% longer just to make sure
No // comments please.
+ unsigned int one_char_receive_duration = (12 * 1000) / (port->uartclk / (1000 * 1000) ); + + /* CT=0xFFFF sets the serial line to the minimal possible baudrate depending on the uartclk. */ + out_8(&psc->ctur, 0xFF); + out_8(&psc->ctlr, 0xFF); + + reset_errors_and_wait( port, unlock, flags, one_char_receive_duration * 2 ); + + while ((in_be16(&psc->mpc52xx_psc_status)) & MPC52xx_PSC_SR_RB) { + reset_errors_and_wait( port, unlock, flags, one_char_receive_duration ); + } +#endif + out_8(&psc->command,MPC52xx_PSC_RST_RX); +} + static int mpc52xx_uart_startup(struct uart_port *port) {@@ -510,7 +563,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(port, false, 0); out_8(&psc->command, MPC52xx_PSC_RST_TX); out_be32(&psc->sicr, 0); /* UART mode DCD ignored */@@ -529,7 +582,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(port, false, 0); if (!uart_console(port)) out_8(&psc->command, MPC52xx_PSC_RST_TX);@@ -588,9 +641,6 @@ mpc52xx_uart_set_termios(struct uart_port *port, struct ktermios *new, /* Get the lock */ spin_lock_irqsave(&port->lock, flags);
Bye, Wolfram -- Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de Pengutronix - Linux Solutions for Science and Industry
From: René Bürgel <hidden>
Date: 2008-11-06 08:10:37
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 When a device with a low baudrate is connected to the serial port, but the processor "listens" on a higher baudrate, it might falsely receive breaks from the controller. During a break, the serial controller may not be reset. The appended patch provides a workaround for that situation by lowering the baudrate without resetting the controller and waiting until no break is received anymore. This is v4 if the patch, just reformatted to fit the linux kernel coding style without functional changes. Wolfram Sang schrieb:
Hi René, On Tue, Nov 04, 2008 at 08:40:09PM +0100, René Bürgel wrotequoted
But there's still one thing, that bothers me a bit - if there is REALLY a break on the line, closing the driver may take until it's gone. I don't know whether this is really satisfying, but i think it's better than the alternative: no serial connection until the next reboot.I think we should CC linux-serial to get some opinions about this. At least, if it stays like this, it should be mentioned in the source.
What's the opinion from the linux-serial folks about this issue? -- René Bürgel Software Engineer Unicontrol Systemtechnik GmbH OT Dittersbach Sachsenburger Weg 34 09669 Frankenberg Tel.: 03 72 06/ 88 73 - 19 Fax: 03 72 06/ 88 73 - 60 E-Mail: r.buergel@unicontrol.de Internet: www.unicontrol.de Unicontrol Systemtechnik GmbH Geschäftsführer: Dipl.-Ing. Siegfried Heinze Sitz der Gesellschaft: Frankenberg Registergericht: Amtsgericht Chemnitz, HRB 15 475 Wichtiger Hinweis: Diese E-Mail und etwaige Anlagen können Betriebs- und Geschäftsgeheimnisse, dem Anwaltsgeheimnis unterliegende oder sonstige vertrauliche Informationen enthalten. Sollten Sie diese E-Mail irrtümlich erhalten haben, ist Ihnen der Status dieser E-Mail bekannt. Bitte benachrichtigen Sie uns in diesem Falle sofort durch Antwort-Mail und löschen Sie diese E-Mail nebst etwaigen Anlagen aus Ihrem System. Ebenso dürfen Sie diese E-Mail oder ihre Anlagen nicht kopieren oder an Dritte weitergeben. Vielen Dank! Important Note: This e-mail and any attachments are confidential, may contain trade secrets and may well also be legally privileged or otherwise protected from disclosure. If you have received it in error, you are on notice of its status. Please notify us immediately by reply e-mail and then delete this e-mail and any attachment from your system. If you are not the intended recipient please understand that you must not copy this e-mail or any attachments or disclose the contents to any other person. Thank you.
From: René Bürgel <hidden>
Date: 2008-11-06 17:02:13
Matt Sealey schrieb:
René Bürgel wrote:quoted
But as the serial driver is also used for the MPC5121, we may have to distinguish anyway. Does anyone have the possibility to test if the bug in still present on MPC5121?Tell us what to do to get it to occur and what we're looking for and we have a bunch of boards at Genesi, someone will find the time to test it (if not them, then me :) Right now I'm having a hell of a time getting a 2.6.27.x kernel running on it though, some driver support is still missing from mainline making it just that little bit extra frustrating to work with.. (if you're using the system over a serial console, how are you supposed to test serial port operation? USB doesn't work in >2.6.24 so I guess I have to hope netconsole works :)
Hi, Matt Thanks for your offer. I'm using telnet to debug the serial port. I'll append a testcase for you, which is just opening a serial port, trying to receive for a second, switching the baudrate and doing it receiving again. Just run it and connect a slow device to the serial port sending data continuously. I'm using a GPS mouse here, working at 9600baud. The higher the rate you are receiving at, the higher the chance to falsely receive a break. When the serial port is switch off in that moment (the filedescriptor is closed), the serial won't receive anything from that time, if the bug is present Alternativly, if you have more control over your serial device, just send breaks continuously, open and close the serial port. Open it again and receiving data fails, if the bug is present. Just btw: if USB is not working, did you miss the initialisation of the USB-controller in your bootloader? I had similar problems getting from 2.6.22 to 2.6.25 with my mpc5200. -- René Bürgel Software Engineer Unicontrol Systemtechnik GmbH OT Dittersbach Sachsenburger Weg 34 09669 Frankenberg Tel.: 03 72 06/ 88 73 - 19 Fax: 03 72 06/ 88 73 - 60 E-Mail: r.buergel@unicontrol.de Internet: www.unicontrol.de Unicontrol Systemtechnik GmbH Geschäftsführer: Dipl.-Ing. Siegfried Heinze Sitz der Gesellschaft: Frankenberg Registergericht: Amtsgericht Chemnitz, HRB 15 475
From: Matt Sealey <hidden>
Date: 2008-11-06 22:41:08
René Bürgel wrote:
Matt Sealey schrieb: Alternativly, if you have more control over your serial device, just send breaks continuously, open and close the serial port. Open it again and receiving data fails, if the bug is present.
Well I have a couple systems I can write data from here, a little app to send data really isn't that hard to come up with.
Just btw: if USB is not working, did you miss the initialisation of the USB-controller in your bootloader? I had similar problems getting from 2.6.22 to 2.6.25 with my mpc5200.
Actually it's more to do with the fact that the patches to get USB working (along with On The Go support) in the LTIB are quite intrusive, and written for 2.6.24 - since then the USB cores seem to have been reshuffled so it is far from a clean patch. Someday I will get bored and port it to 2.6.28 :) -- Matt Sealey [off-list ref] Genesi, Manager, Developer Relations
From: Grant Likely <hidden>
Date: 2008-11-14 19:09:55
On Thu, Nov 6, 2008 at 1:11 AM, Ren=E9 B=FCrgel [off-list ref] w= rote:
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=3D1&WT_=
TYPE=3DErrata&WT_VENDOR=3DFREESCALE&WT_FILE_FORMAT=3Dpdf&WT_ASSET=3DDocumen= tation
When a device with a low baudrate is connected to the serial port, but th=
e
processor "listens" on a higher baudrate, it might falsely receive breaks from the controller. During a break, the serial controller may not be res=
et.
The appended patch provides a workaround for that situation by lowering t=
he
baudrate without resetting the controller and waiting until no break is received anymore.
I'm still don't like the state of this patch for two reasons: 1. It is enabled/disabled by a #ifdef. It is quite possible that we will eventually be able to build a multiplaform kernel that boots on both mpc5200 and mpc5121. The workaround needs to be detected and enabled at runtime based on the data in the device tree (ie. if the compatible property is "fsl,mpc5200-psc-uart"). 2. I'm do not like the mdelay() busywait loop. The long busy wait just wastes CPU time. Doing it with IRQs off means that irq latencies become unbounded while this is happening. This needs to be reworked to use a workqueue or something similar. Also, I'm not convinced that this is the best fix. It doesn't actually solve the problem, it just makes it less likely to occur. What happens if you instead manipulate portconfig to change the PSC pins to GPIO? I wonder if that will disconnect the RX pin from the PSC entirely. If it does, then that might be a suitable method to eliminate the break condition entirely. g. --=20 Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.