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

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

[René Bürgel]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

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



[-- Attachment #2: 122-mpc52xx_erratum_364.patch --]
[-- Type: text/plain, Size: 2924 bytes --]

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);

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

[Grant Likely]

On Mon, Nov 3, 2008 at 12:32 PM, Ren=E9 B=FCrgel <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=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.

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

[Matt Sealey]

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.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

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

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Grant Likely]

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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

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

[René Bürgel]

[-- Attachment #1: Type: text/plain, Size: 3838 bytes --]

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 :)

-- 
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



[-- Attachment #2: 123-mpc52xx_erratum_364.patch --]
[-- Type: text/plain, Size: 3019 bytes --]

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);

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3891 bytes --]

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

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[René Bürgel]

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

-- 
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

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

[Matt Sealey]

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)

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

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

[Matt Sealey]

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 <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [PATCH V3] workaround for mpc52xx erratum #364 (serial may not be reset in break state)

[René Bürgel]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

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



[-- Attachment #2: 130-mpc52xx_erratum_364.patch --]
[-- Type: text/plain, Size: 3916 bytes --]

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.
+ */
+
+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);
+		}
+	}
+
+
+	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
+	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);
 
-	/* 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 +657,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);

Re: [PATCH V3] workaround for mpc52xx erratum #364 (serial may not be reset in break state)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 6018 bytes --]

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);
>  
> -	/* 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 +657,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);

Bye,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH V4] workaround for mpc52xx erratum #364 (serial may not be reset in break state)

[René Bürgel]

[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]

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 wrote
>> 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.



[-- Attachment #2: 127-mpc52xx_erratum_364.patch --]
[-- Type: text/plain, Size: 3994 bytes --]

diff --git a/drivers/serial/mpc52xx_uart.c b/drivers/serial/mpc52xx_uart.c
index 6117d3d..ae539b5 100644
--- a/drivers/serial/mpc52xx_uart.c
+++ b/drivers/serial/mpc52xx_uart.c
@@ -496,6 +496,74 @@ 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.
+ */
+
+#ifdef CONFIG_PPC_MPC52xx
+static 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);
+	}
+}
+#endif
+
+static void mpc52xx_uart_reset_rx(struct uart_port *port, bool unlock,
+				  unsigned long flags)
+{
+#ifdef CONFIG_PPC_MPC52xx
+	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  / (port->uartclk / (1000 * 1000) ) * 1000,
+	 * (bits)                 (MHz -> Hz)      (s -> ms)
+	 */
+	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 +578,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 +597,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 +656,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 +672,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);

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

[René Bürgel]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

Matt Sealey schrieb:
>
>
> 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 :)
>
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


[-- Attachment #2: erratum364-tc.c --]
[-- Type: text/plain, Size: 3055 bytes --]

#include <linux/serial.h>
#include <poll.h>
#include <fcntl.h>
#include <termios.h>
#include <sys/ioctl.h>
#include <stdio.h>
#include <errno.h>
#include <unistd.h>
#include <string.h>

#define DEV_UART "/dev/tts/0"


void initUart(int uart, struct pollfd* pfd, speed_t speed, struct serial_icounter_struct* last_icounter)
{
  struct termios options;
  tcgetattr( uart, &options );

  cfmakeraw( &options );
  cfsetispeed( &options, speed );
  cfsetospeed( &options, speed );

  options.c_cflag &= ~CSIZE;    //clear charsize-bits
  options.c_cflag |= CS8;
  options.c_iflag &= ~PARENB;
  options.c_cflag &= ~CSTOPB;
  options.c_iflag &= ~IGNPAR; // do not ignore framing and parity errors

  tcsetattr( uart, TCSANOW, &options );
  tcflush( uart, TCIOFLUSH );

  //read garbage
  if (poll( pfd, 1, 1000 ) > 0)
  {
    usleep( 500 /*ms*/ * 1000 /*us*/ );

    unsigned char buffer[16384 + 1];
    read( uart, buffer, sizeof( buffer ) );
    ioctl( uart, TIOCGICOUNT, last_icounter );
  }
}

void readUart(int uart, struct pollfd* pfd, struct serial_icounter_struct* last_icounter)
{
  int retval = poll( pfd, 1, 1000 );

  if ( retval > 0 )
  {
    if ( pfd->revents & POLLIN )
    {
      // etwas warten, damit nicht ständig einzelne Zeichen von der Seriellen geholt werden
      // TODO: für z.B. A/D-Probe ggf. reduzieren (Template-Argument?)
      usleep( 2 /*ms*/ * 1000 /*us*/ );

      unsigned buffer[16384 + 1];
      int countBytes = read( uart, buffer, sizeof( buffer ) );

      printf("received something: ");
      {
        int i = 0;
        for (;  i < countBytes; ++i)
          printf("%x ", (unsigned short) buffer[i]);
      }
      printf("\n");

      struct serial_icounter_struct icounter;
      ioctl( uart, TIOCGICOUNT, &icounter );

      if (icounter.brk     != last_icounter->brk    ) printf("Break ");
      if (icounter.frame   != last_icounter->frame  ) printf("Framing Error ");
      if (icounter.parity  != last_icounter->parity ) printf("Parity Error ");
      if (icounter.overrun != last_icounter->overrun) printf("Overrun");
      printf("\n");

      *last_icounter = icounter;
    }
    else
    {
      printf("received nothing\n");
    }
  }
  else
  {
	int error = errno;
	printf("error while waiting for data on serial port: %s\n", strerror( error ) );
  }
}

speed_t getNextSpeed(speed_t curSpeed)
{
	switch (curSpeed)
        {
          case   B4800: return   B9600;
          case   B9600: return  B19200;
          case  B19200: return  B38400;
          case  B38400: return  B57600;
          case  B57600: return B115200;
          case B115200: return   B4800;
        }
	return B4800;
}

int main()
{
	struct pollfd pfd;
	pfd.events = POLLIN;
	speed_t cur_speed=B4800;
        struct serial_icounter_struct last_icounter;

	while(1)
	{
		pfd.fd = open( DEV_UART, O_RDWR | O_NONBLOCK );
		
		printf("trying %d\n", cur_speed);
		initUart(pfd.fd, &pfd, cur_speed, &last_icounter);
		readUart(pfd.fd, &pfd, &last_icounter );
		close( pfd.fd );
		cur_speed = getNextSpeed( cur_speed );
	}
	return 0;
}

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

[Matt Sealey]

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 <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [PATCH V4] workaround for mpc52xx erratum #364 (serial may not be reset in break state)

[Grant Likely]

On Thu, Nov 6, 2008 at 1:11 AM, Ren=E9 B=FCrgel <r.buergel@unicontrol.de> 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.