Exar ST16C2550 rev A2 bug

Exar ST16C2550 rev A2 bug

[Alex Williamson]

Hi Russell,

   There seem to be an increasing number of the above UARTs floating
around and I'm wondering if we can do something to better detect and
work around their flaw.  Exar has documented the problem and their
proposed serial driver changes to work around the issue here:

http://www.exar.com/info.php?pdf=dan180_oct2004.pdf

In short, they had a mask problem that inadvertently exposes the DVID,
DREV and EFR registers on the chip.  This causes us to detect the device
as a 16650V2 and try to make use of the FIFO as if it were 32 bytes.
The previous version of the UART detected correctly as a 16550A and
worked fine.  Exar's proposed solution is simply to only set the port
type to 16650V2 if size_fifo() == 32, or simply:

===== drivers/serial/8250.c 1.91 vs edited =====
--- 1.91/drivers/serial/8250.c  2004-11-13 17:43:56 -07:00
+++ edited/drivers/serial/8250.c        2004-11-17 11:13:05 -07:00
@@ -570,7 +570,7 @@
         */
        if (size_fifo(up) == 64)
                up->port.type = PORT_16654;
-       else
+       else if (size_fifo(up) == 32)
                up->port.type = PORT_16650V2;
 }
 
The proposed 2.4 solution is quite similar.  It seems reasonable to
verify the reported FIFO size, but perhaps you have a better solution.
For anyone currently impacted by this bug, a simple yet limited
workaround is to change the uart type using setserial.  Thanks,

	Alex

-- 
Alex Williamson                             HP Linux & Open Source Lab

Re: Exar ST16C2550 rev A2 bug

[Russell King]

On Wed, Nov 17, 2004 at 11:26:47AM -0700, Alex Williamson wrote:
>    There seem to be an increasing number of the above UARTs floating
> around and I'm wondering if we can do something to better detect and
> work around their flaw.  Exar has documented the problem and their
> proposed serial driver changes to work around the issue here:
> 
> http://www.exar.com/info.php?pdf=dan180_oct2004.pdf

Can you check whether this patch solves it?  I'd rather not rely on
size_fifo() to do the right thing in every circumstance.

Essentially, if we find an EFR, we check whether the UART reports
DVID/DREV values corresponding to the known problem scenario, and
if so, we essentially ignore the EFR.

What I don't know is whether these DVID/DREV values correspond to
a real device which does have an EFR.  Maybe Exar people can shed
some light on this?

(Also, one has to wonder what information Exar has about what we're
working on, which we don't know ourselves about... check out the
above link, FAQ question 6. 8))

===== drivers/serial/8250.c 1.92 vs edited =====
--- 1.92/drivers/serial/8250.c	2004-11-19 07:03:10 +00:00
+++ edited/drivers/serial/8250.c	2004-11-28 11:02:32 +00:00
@@ -479,6 +479,34 @@
 }
 
 /*
+ * Read UART ID using the divisor method - set DLL and DLM to zero
+ * and the revision will be in DLL and device type in DLM.  We
+ * preserve the device state across this.
+ */
+static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
+{
+	unsigned char old_dll, old_dlm, old_lcr;
+	unsigned int id;
+
+	old_lcr = serial_inp(p, UART_LCR);
+	serial_outp(p, UART_LCR, UART_LCR_DLAB);
+
+	old_dll = serial_inp(p, UART_DLL);
+	old_dlm = serial_inp(p, UART_DLM);
+
+	serial_outp(p, UART_DLL, 0);
+	serial_outp(p, UART_DLM, 0);
+
+	id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8;
+
+	serial_outp(p, UART_DLL, old_dll);
+	serial_outp(p, UART_DLM, old_dlm);
+	serial_outp(p, UART_LCR, old_lcr);
+
+	return id;
+}
+
+/*
  * This is a helper routine to autodetect StarTech/Exar/Oxsemi UART's.
  * When this function is called we know it is at least a StarTech
  * 16650 V2, but it might be one of several StarTech UARTs, or one of
@@ -490,7 +518,7 @@
  */
 static void autoconfig_has_efr(struct uart_8250_port *up)
 {
-	unsigned char id1, id2, id3, rev, saved_dll, saved_dlm;
+	unsigned int id1, id2, id3, rev;
 
 	/*
 	 * Everything with an EFR has SLEEP
@@ -540,21 +568,13 @@
 	 *  0x12 - XR16C2850.
 	 *  0x14 - XR16C854.
 	 */
-	serial_outp(up, UART_LCR, UART_LCR_DLAB);
-	saved_dll = serial_inp(up, UART_DLL);
-	saved_dlm = serial_inp(up, UART_DLM);
-	serial_outp(up, UART_DLL, 0);
-	serial_outp(up, UART_DLM, 0);
-	id2 = serial_inp(up, UART_DLL);
-	id1 = serial_inp(up, UART_DLM);
-	serial_outp(up, UART_DLL, saved_dll);
-	serial_outp(up, UART_DLM, saved_dlm);
-
-	DEBUG_AUTOCONF("850id=%02x:%02x ", id1, id2);
-
-	if (id1 == 0x10 || id1 == 0x12 || id1 == 0x14) {
-		if (id1 == 0x10)
-			up->rev = id2;
+	id1 = autoconfig_read_divisor_id(up);
+	DEBUG_AUTOCONF("850id=%04x ", id1);
+
+	id2 = id1 >> 8;
+	if (id2 == 0x10 || id2 == 0x12 || id2 == 0x14) {
+		if (id2 == 0x10)
+			up->rev = id1 & 255;
 		up->port.type = PORT_16850;
 		return;
 	}
@@ -634,8 +654,16 @@
 	serial_outp(up, UART_LCR, 0xBF);
 	if (serial_in(up, UART_EFR) == 0) {
 		DEBUG_AUTOCONF("EFRv2 ");
-		autoconfig_has_efr(up);
-		return;
+
+		/*
+		 * Exar ST16C2550 "A2" devices incorrectly detect as
+		 * having an EFR, and report an ID of 0x0201.  See
+		 * http://www.exar.com/info.php?pdf=dan180_oct2004.pdf
+		 */
+		if (autoconfig_read_divisor_id(up) != 0x0201) {
+			autoconfig_has_efr(up);
+			return;
+		}
 	}
 
 	/*


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: Exar ST16C2550 rev A2 bug

[Alex Williamson]

On Sun, 2004-11-28 at 11:10 +0000, Russell King wrote:
> On Wed, Nov 17, 2004 at 11:26:47AM -0700, Alex Williamson wrote:
> >    There seem to be an increasing number of the above UARTs floating
> > around and I'm wondering if we can do something to better detect and
> > work around their flaw.  Exar has documented the problem and their
> > proposed serial driver changes to work around the issue here:
> > 
> > http://www.exar.com/info.php?pdf=dan180_oct2004.pdf
> 
> Can you check whether this patch solves it?  I'd rather not rely on
> size_fifo() to do the right thing in every circumstance.

  I have a system on order with one of these bad UARTs.  I'll be happy
to check the patch when it arrives (hopefully this week).

> Essentially, if we find an EFR, we check whether the UART reports
> DVID/DREV values corresponding to the known problem scenario, and
> if so, we essentially ignore the EFR.
> 
> What I don't know is whether these DVID/DREV values correspond to
> a real device which does have an EFR.  Maybe Exar people can shed
> some light on this?

   I'll see if I can get an engineering contact from Exar to confirm.

> (Also, one has to wonder what information Exar has about what we're
> working on, which we don't know ourselves about... check out the
> above link, FAQ question 6. 8))

  ;^)  Thanks,

	Alex

> ===== drivers/serial/8250.c 1.92 vs edited =====
> --- 1.92/drivers/serial/8250.c	2004-11-19 07:03:10 +00:00
> +++ edited/drivers/serial/8250.c	2004-11-28 11:02:32 +00:00
> @@ -479,6 +479,34 @@
>  }
>  
>  /*
> + * Read UART ID using the divisor method - set DLL and DLM to zero
> + * and the revision will be in DLL and device type in DLM.  We
> + * preserve the device state across this.
> + */
> +static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
> +{
> +	unsigned char old_dll, old_dlm, old_lcr;
> +	unsigned int id;
> +
> +	old_lcr = serial_inp(p, UART_LCR);
> +	serial_outp(p, UART_LCR, UART_LCR_DLAB);
> +
> +	old_dll = serial_inp(p, UART_DLL);
> +	old_dlm = serial_inp(p, UART_DLM);
> +
> +	serial_outp(p, UART_DLL, 0);
> +	serial_outp(p, UART_DLM, 0);
> +
> +	id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8;
> +
> +	serial_outp(p, UART_DLL, old_dll);
> +	serial_outp(p, UART_DLM, old_dlm);
> +	serial_outp(p, UART_LCR, old_lcr);
> +
> +	return id;
> +}
> +
> +/*
>   * This is a helper routine to autodetect StarTech/Exar/Oxsemi UART's.
>   * When this function is called we know it is at least a StarTech
>   * 16650 V2, but it might be one of several StarTech UARTs, or one of
> @@ -490,7 +518,7 @@
>   */
>  static void autoconfig_has_efr(struct uart_8250_port *up)
>  {
> -	unsigned char id1, id2, id3, rev, saved_dll, saved_dlm;
> +	unsigned int id1, id2, id3, rev;
>  
>  	/*
>  	 * Everything with an EFR has SLEEP
> @@ -540,21 +568,13 @@
>  	 *  0x12 - XR16C2850.
>  	 *  0x14 - XR16C854.
>  	 */
> -	serial_outp(up, UART_LCR, UART_LCR_DLAB);
> -	saved_dll = serial_inp(up, UART_DLL);
> -	saved_dlm = serial_inp(up, UART_DLM);
> -	serial_outp(up, UART_DLL, 0);
> -	serial_outp(up, UART_DLM, 0);
> -	id2 = serial_inp(up, UART_DLL);
> -	id1 = serial_inp(up, UART_DLM);
> -	serial_outp(up, UART_DLL, saved_dll);
> -	serial_outp(up, UART_DLM, saved_dlm);
> -
> -	DEBUG_AUTOCONF("850id=%02x:%02x ", id1, id2);
> -
> -	if (id1 == 0x10 || id1 == 0x12 || id1 == 0x14) {
> -		if (id1 == 0x10)
> -			up->rev = id2;
> +	id1 = autoconfig_read_divisor_id(up);
> +	DEBUG_AUTOCONF("850id=%04x ", id1);
> +
> +	id2 = id1 >> 8;
> +	if (id2 == 0x10 || id2 == 0x12 || id2 == 0x14) {
> +		if (id2 == 0x10)
> +			up->rev = id1 & 255;
>  		up->port.type = PORT_16850;
>  		return;
>  	}
> @@ -634,8 +654,16 @@
>  	serial_outp(up, UART_LCR, 0xBF);
>  	if (serial_in(up, UART_EFR) == 0) {
>  		DEBUG_AUTOCONF("EFRv2 ");
> -		autoconfig_has_efr(up);
> -		return;
> +
> +		/*
> +		 * Exar ST16C2550 "A2" devices incorrectly detect as
> +		 * having an EFR, and report an ID of 0x0201.  See
> +		 * http://www.exar.com/info.php?pdf=dan180_oct2004.pdf
> +		 */
> +		if (autoconfig_read_divisor_id(up) != 0x0201) {
> +			autoconfig_has_efr(up);
> +			return;
> +		}
>  	}
>  
>  	/*
> 

Re: Exar ST16C2550 rev A2 bug

[Alex Williamson]

Russell,

On Sun, 2004-11-28 at 09:26 -0700, Alex Williamson wrote:
> On Sun, 2004-11-28 at 11:10 +0000, Russell King wrote:
> > On Wed, Nov 17, 2004 at 11:26:47AM -0700, Alex Williamson wrote:
> > 
> > Can you check whether this patch solves it?  I'd rather not rely on
> > size_fifo() to do the right thing in every circumstance.
> 
>   I have a system on order with one of these bad UARTs.  I'll be happy
> to check the patch when it arrives (hopefully this week).

   No system yet, hopefully soon :^(

> > Essentially, if we find an EFR, we check whether the UART reports
> > DVID/DREV values corresponding to the known problem scenario, and
> > if so, we essentially ignore the EFR.
> > 
> > What I don't know is whether these DVID/DREV values correspond to
> > a real device which does have an EFR.  Maybe Exar people can shed
> > some light on this?
> 
>    I'll see if I can get an engineering contact from Exar to confirm.

   Michael Knight from Exar is CC'd.  Unfortunately it sounds like the
DVID/DREV registers are the same as another Exar product that does have
a valid EFR.  So this would restrict functionality on that part as well.
Perhaps a hybrid test, something like:

static int broken_efr(struct uart_8250_port *p)
{
	/*
	 * Exar ST16C2550 "A2" devices incorrectly detect as
	 * having an EFR, and report an ID of 0x0201.  See
	 * http://www.exar.com/info.php?pdf=dan180_oct2004.pdf
	 */
	if (autoconfig_read_divisor_id(p) == 0x0201 && size_fifo(p) == 16)
		return 1;

	return 0;
}

...

	if (!broken_efr(up)) {
		autoconfig_has_efr(up);
		return;
	}
...

   I think we know size_fifo() works on these parts, so we'd limit our
reliance on size_fifo().  If any more UARTs come alone with this issue
we could add them to the test.  Thoughts?  I can incorporate this into
your patch and test it when I get a system with these parts.  Thanks,

	Alex


> > ===== drivers/serial/8250.c 1.92 vs edited =====
> > --- 1.92/drivers/serial/8250.c	2004-11-19 07:03:10 +00:00
> > +++ edited/drivers/serial/8250.c	2004-11-28 11:02:32 +00:00
> > @@ -479,6 +479,34 @@
> >  }
> >  
> >  /*
> > + * Read UART ID using the divisor method - set DLL and DLM to zero
> > + * and the revision will be in DLL and device type in DLM.  We
> > + * preserve the device state across this.
> > + */
> > +static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
> > +{
> > +	unsigned char old_dll, old_dlm, old_lcr;
> > +	unsigned int id;
> > +
> > +	old_lcr = serial_inp(p, UART_LCR);
> > +	serial_outp(p, UART_LCR, UART_LCR_DLAB);
> > +
> > +	old_dll = serial_inp(p, UART_DLL);
> > +	old_dlm = serial_inp(p, UART_DLM);
> > +
> > +	serial_outp(p, UART_DLL, 0);
> > +	serial_outp(p, UART_DLM, 0);
> > +
> > +	id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8;
> > +
> > +	serial_outp(p, UART_DLL, old_dll);
> > +	serial_outp(p, UART_DLM, old_dlm);
> > +	serial_outp(p, UART_LCR, old_lcr);
> > +
> > +	return id;
> > +}
> > +
> > +/*
> >   * This is a helper routine to autodetect StarTech/Exar/Oxsemi UART's.
> >   * When this function is called we know it is at least a StarTech
> >   * 16650 V2, but it might be one of several StarTech UARTs, or one of
> > @@ -490,7 +518,7 @@
> >   */
> >  static void autoconfig_has_efr(struct uart_8250_port *up)
> >  {
> > -	unsigned char id1, id2, id3, rev, saved_dll, saved_dlm;
> > +	unsigned int id1, id2, id3, rev;
> >  
> >  	/*
> >  	 * Everything with an EFR has SLEEP
> > @@ -540,21 +568,13 @@
> >  	 *  0x12 - XR16C2850.
> >  	 *  0x14 - XR16C854.
> >  	 */
> > -	serial_outp(up, UART_LCR, UART_LCR_DLAB);
> > -	saved_dll = serial_inp(up, UART_DLL);
> > -	saved_dlm = serial_inp(up, UART_DLM);
> > -	serial_outp(up, UART_DLL, 0);
> > -	serial_outp(up, UART_DLM, 0);
> > -	id2 = serial_inp(up, UART_DLL);
> > -	id1 = serial_inp(up, UART_DLM);
> > -	serial_outp(up, UART_DLL, saved_dll);
> > -	serial_outp(up, UART_DLM, saved_dlm);
> > -
> > -	DEBUG_AUTOCONF("850id=%02x:%02x ", id1, id2);
> > -
> > -	if (id1 == 0x10 || id1 == 0x12 || id1 == 0x14) {
> > -		if (id1 == 0x10)
> > -			up->rev = id2;
> > +	id1 = autoconfig_read_divisor_id(up);
> > +	DEBUG_AUTOCONF("850id=%04x ", id1);
> > +
> > +	id2 = id1 >> 8;
> > +	if (id2 == 0x10 || id2 == 0x12 || id2 == 0x14) {
> > +		if (id2 == 0x10)
> > +			up->rev = id1 & 255;
> >  		up->port.type = PORT_16850;
> >  		return;
> >  	}
> > @@ -634,8 +654,16 @@
> >  	serial_outp(up, UART_LCR, 0xBF);
> >  	if (serial_in(up, UART_EFR) == 0) {
> >  		DEBUG_AUTOCONF("EFRv2 ");
> > -		autoconfig_has_efr(up);
> > -		return;
> > +
> > +		/*
> > +		 * Exar ST16C2550 "A2" devices incorrectly detect as
> > +		 * having an EFR, and report an ID of 0x0201.  See
> > +		 * http://www.exar.com/info.php?pdf=dan180_oct2004.pdf
> > +		 */
> > +		if (autoconfig_read_divisor_id(up) != 0x0201) {
> > +			autoconfig_has_efr(up);
> > +			return;
> > +		}
> >  	}
> >  
> >  	/*
> > 
> 
-- 
Alex Williamson                             HP Linux & Open Source Lab

Re: Exar ST16C2550 rev A2 bug

[Alex Williamson]

Hi Russell,

   I received a system with one of these misidentified UARTs we were
discussing.  The proposed addition to your solution works almost as
advertised.  The only trick is that size_fifo() doesn't like coming in
with 0xBF in the LCR.  I added a simple save/clear/restore and all seems
well.  This probably needs to be checked since I have a very limited
assortment of UARTs to test.  The complete patch is attached below.  I'd
be more than happy to run any additional test cases on this system.
Please let me know if this looks reasonable.  Thanks,

	Alex

-- 
Alex Williamson                             HP Linux & Open Source Lab

===== drivers/serial/8250.c 1.92 vs edited =====
--- 1.92/drivers/serial/8250.c	2004-11-19 00:03:10 -07:00
+++ edited/drivers/serial/8250.c	2004-12-13 14:57:48 -07:00
@@ -449,9 +449,11 @@
  */
 static int size_fifo(struct uart_8250_port *up)
 {
-	unsigned char old_fcr, old_mcr, old_dll, old_dlm;
+	unsigned char old_fcr, old_mcr, old_dll, old_dlm, old_lcr;
 	int count;
 
+	old_lcr = serial_inp(up, UART_LCR);
+	serial_outp(up, UART_LCR, 0);
 	old_fcr = serial_inp(up, UART_FCR);
 	old_mcr = serial_inp(up, UART_MCR);
 	serial_outp(up, UART_FCR, UART_FCR_ENABLE_FIFO |
@@ -474,11 +476,40 @@
 	serial_outp(up, UART_LCR, UART_LCR_DLAB);
 	serial_outp(up, UART_DLL, old_dll);
 	serial_outp(up, UART_DLM, old_dlm);
+	serial_outp(up, UART_LCR, old_lcr);
 
 	return count;
 }
 
 /*
+ * Read UART ID using the divisor method - set DLL and DLM to zero
+ * and the revision will be in DLL and device type in DLM.  We
+ * preserve the device state across this.
+ */
+static unsigned int autoconfig_read_divisor_id(struct uart_8250_port *p)
+{
+	unsigned char old_dll, old_dlm, old_lcr;
+	unsigned int id;
+
+	old_lcr = serial_inp(p, UART_LCR);
+	serial_outp(p, UART_LCR, UART_LCR_DLAB);
+
+	old_dll = serial_inp(p, UART_DLL);
+	old_dlm = serial_inp(p, UART_DLM);
+
+	serial_outp(p, UART_DLL, 0);
+	serial_outp(p, UART_DLM, 0);
+
+	id = serial_inp(p, UART_DLL) | serial_inp(p, UART_DLM) << 8;
+
+	serial_outp(p, UART_DLL, old_dll);
+	serial_outp(p, UART_DLM, old_dlm);
+	serial_outp(p, UART_LCR, old_lcr);
+
+	return id;
+}
+
+/*
  * This is a helper routine to autodetect StarTech/Exar/Oxsemi UART's.
  * When this function is called we know it is at least a StarTech
  * 16650 V2, but it might be one of several StarTech UARTs, or one of
@@ -490,7 +521,7 @@
  */
 static void autoconfig_has_efr(struct uart_8250_port *up)
 {
-	unsigned char id1, id2, id3, rev, saved_dll, saved_dlm;
+	unsigned int id1, id2, id3, rev;
 
 	/*
 	 * Everything with an EFR has SLEEP
@@ -540,21 +571,13 @@
 	 *  0x12 - XR16C2850.
 	 *  0x14 - XR16C854.
 	 */
-	serial_outp(up, UART_LCR, UART_LCR_DLAB);
-	saved_dll = serial_inp(up, UART_DLL);
-	saved_dlm = serial_inp(up, UART_DLM);
-	serial_outp(up, UART_DLL, 0);
-	serial_outp(up, UART_DLM, 0);
-	id2 = serial_inp(up, UART_DLL);
-	id1 = serial_inp(up, UART_DLM);
-	serial_outp(up, UART_DLL, saved_dll);
-	serial_outp(up, UART_DLM, saved_dlm);
-
-	DEBUG_AUTOCONF("850id=%02x:%02x ", id1, id2);
-
-	if (id1 == 0x10 || id1 == 0x12 || id1 == 0x14) {
-		if (id1 == 0x10)
-			up->rev = id2;
+	id1 = autoconfig_read_divisor_id(up);
+	DEBUG_AUTOCONF("850id=%04x ", id1);
+
+	id2 = id1 >> 8;
+	if (id2 == 0x10 || id2 == 0x12 || id2 == 0x14) {
+		if (id2 == 0x10)
+			up->rev = id1 & 255;
 		up->port.type = PORT_16850;
 		return;
 	}
@@ -596,6 +619,19 @@
 		up->port.type = PORT_16450;
 }
 
+static int broken_efr(struct uart_8250_port *up)
+{
+	/*
+	 * Exar ST16C2550 "A2" devices incorrectly detect as
+	 * having an EFR, and report an ID of 0x0201.  See
+	 * http://www.exar.com/info.php?pdf=dan180_oct2004.pdf
+	 */
+	if (autoconfig_read_divisor_id(up) == 0x0201 && size_fifo(up) == 16)
+		return 1;
+
+	return 0;
+}
+
 /*
  * We know that the chip has FIFOs.  Does it have an EFR?  The
  * EFR is located in the same register position as the IIR and
@@ -632,7 +668,7 @@
 	 * (other ST16C650V2 UARTs, TI16C752A, etc)
 	 */
 	serial_outp(up, UART_LCR, 0xBF);
-	if (serial_in(up, UART_EFR) == 0) {
+	if (serial_in(up, UART_EFR) == 0 && !broken_efr(up)) {
 		DEBUG_AUTOCONF("EFRv2 ");
 		autoconfig_has_efr(up);
 		return;