diff mbox

[V2] workaround for mpc52xx erratum #364 (serial may not be reset in break state)

Message ID 4910274E.5030305@unicontrol.de (mailing list archive)
State Superseded, archived
Delegated to: Grant Likely
Headers show

Commit Message

René Bürgel Nov. 4, 2008, 10:43 a.m. UTC 
Grant Likely schrieb:
> 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.
>
>   
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.
>> 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.
>
>   
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 :)

Comments

Wolfram Sang Nov. 4, 2008, 11:15 a.m. UTC | #1 
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);
>  
> -	/* 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 +635,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(port, true, flags);
>  	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);

> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Thanks!

   Wolfram Sang
René Bürgel Nov. 4, 2008, 2:13 p.m. UTC | #2 
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 :)
>   
>> + *
>> + * 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?
>   
Yes, currently it doesn't work when receiving with less than 4800 baud. 
I'll try to fix this.
>   
>> +{
>> +	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
Matt Sealey Nov. 4, 2008, 6:23 p.m. UTC | #3 
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 Nov. 6, 2008, 10:41 p.m. UTC | #4 
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 :)
diff mbox

Patch

diff --git a/drivers/serial/mpc52xx_uart.c b/drivers/serial/mpc52xx_uart.c
index 6117d3d..6af51b8 100644
--- a/drivers/serial/mpc52xx_uart.c
+++ b/drivers/serial/mpc52xx_uart.c
@@ -496,6 +496,37 @@  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
+ */
+
+static void mpc52xx_uart_reset_rx(struct uart_port *port, int unlock, unsigned long flags)
+{
+	struct mpc52xx_psc __iomem *psc = PSC(port);
+#ifdef CONFIG_PPC_MPC52xx
+	out_8(&psc->ctur,0x01);
+	out_8(&psc->ctlr,0xae);
+	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);
 
-	/* 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 +635,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(port, true, flags);
 	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);