[PATCH] drivers/char/serial.c bug in ST16C654 detection

[PATCH] drivers/char/serial.c bug in ST16C654 detection

[Val Henson]

This fixes a bug in the autoconfig_startech_uarts function in
serial.c.  The problem is that 0's are written to the baud rate
registers in order to detect an XR16C850 or XR16C854.  This makes the
Exar ST16C654 go kablooey.  Saving and restoring the baud rate
registers after the test fixes it.

I'm assuming that the XR16C85[04] detection works as is and doesn't
need the original baud rate restored.  If I'm wrong, I'll rewrite the
patch.

-VAL

--- linux-2.4.5-pre1/drivers/char/serial.c	Thu Apr 19 00:26:34 2001
+++ linux/drivers/char/serial.c	Sat May 12 05:19:26 2001
@@ -3507,7 +3507,7 @@
 				      struct serial_state *state,
 				      unsigned long flags)
 {
-	unsigned char scratch, scratch2, scratch3;
+	unsigned char scratch, scratch2, scratch3, scratch4;
 
 	/*
 	 * First we check to see if it's an Oxford Semiconductor UART.
@@ -3551,17 +3551,32 @@
 	 * XR16C854.
 	 * 
 	 */
+
+	/* Save the DLL and DLM */
+
 	serial_outp(info, UART_LCR, UART_LCR_DLAB);
+	scratch3 = serial_inp(info, UART_DLL);
+	scratch4 = serial_inp(info, UART_DLM);
+
 	serial_outp(info, UART_DLL, 0);
 	serial_outp(info, UART_DLM, 0);
-	state->revision = serial_inp(info, UART_DLL);
+	scratch2 = serial_inp(info, UART_DLL);
 	scratch = serial_inp(info, UART_DLM);
 	serial_outp(info, UART_LCR, 0);
+
 	if (scratch == 0x10 || scratch == 0x14) {
+		if (scratch == 0x10)
+			state->revision = scratch2;
 		state->type = PORT_16850;
 		return;
 	}
 
+	/* Restore the DLL and DLM */
+
+	serial_outp(info, UART_LCR, UART_LCR_DLAB);
+	serial_outp(info, UART_DLL, scratch3);
+	serial_outp(info, UART_DLM, scratch4);
+	serial_outp(info, UART_LCR, 0);
 	/*
 	 * We distinguish between the '654 and the '650 by counting
 	 * how many bytes are in the FIFO.  I'm using this for now,

Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection

[Stuart MacDonald]

From: "Val Henson" <val@nmt.edu>
> This fixes a bug in the autoconfig_startech_uarts function in
> serial.c.  The problem is that 0's are written to the baud rate
> registers in order to detect an XR16C850 or XR16C854.  This makes the
> Exar ST16C654 go kablooey.  Saving and restoring the baud rate
> registers after the test fixes it.

What version of serial.c? I'm assuming 5.05.

Define "go kablooey". We haven't noticed any problems, and we supplied
this bit of code.

The size_fifo() routine supplies its own baud rate divisor, and
on any rs_open() change_speed() sets the baud rate properly.
I can't figure what you might be seeing.

> I'm assuming that the XR16C85[04] detection works as is and doesn't
> need the original baud rate restored.  If I'm wrong, I'll rewrite the
> patch.

You're correct that the detection works as is, except that you
broke it in your patch. See below.

> --- linux-2.4.5-pre1/drivers/char/serial.c Thu Apr 19 00:26:34 2001
> +++ linux/drivers/char/serial.c Sat May 12 05:19:26 2001
> @@ -3507,7 +3507,7 @@
>         struct serial_state *state,
>         unsigned long flags)
>  {
> - unsigned char scratch, scratch2, scratch3;
> + unsigned char scratch, scratch2, scratch3, scratch4;
>
>   /*
>   * First we check to see if it's an Oxford Semiconductor UART.
> @@ -3551,17 +3551,32 @@
>   * XR16C854.
>   *
>   */
> +
> + /* Save the DLL and DLM */
> +
>   serial_outp(info, UART_LCR, UART_LCR_DLAB);
> + scratch3 = serial_inp(info, UART_DLL);
> + scratch4 = serial_inp(info, UART_DLM);
> +
>   serial_outp(info, UART_DLL, 0);
>   serial_outp(info, UART_DLM, 0);
> - state->revision = serial_inp(info, UART_DLL);
> + scratch2 = serial_inp(info, UART_DLL);

This isn't necessary. The revision field is only checked
for 950s, so setting it here doesn't harm anything. If the
current (only) example of checking it is followed as normal
procedure, the port type will always be checked first, before
checking the revision, ensuring only valid revisions are
referenced.

>   scratch = serial_inp(info, UART_DLM);
>   serial_outp(info, UART_LCR, 0);
> +
>   if (scratch == 0x10 || scratch == 0x14) {
> + if (scratch == 0x10)
> + state->revision = scratch2;
>   state->type = PORT_16850;
>   return;
>   }

Only saving the revision for 850s is probably wrong. It should
be saved for all the 85x uarts.

> + /* Restore the DLL and DLM */
> +
> + serial_outp(info, UART_LCR, UART_LCR_DLAB);
> + serial_outp(info, UART_DLL, scratch3);
> + serial_outp(info, UART_DLM, scratch4);
> + serial_outp(info, UART_LCR, 0);
>   /*
>   * We distinguish between the '654 and the '650 by counting
>   * how many bytes are in the FIFO.  I'm using this for now,

..Stu

Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection

[Val Henson]

On Mon, May 14, 2001 at 03:50:01PM -0400, Stuart MacDonald wrote:

> What version of serial.c? I'm assuming 5.05.

Serial driver version 5.05a (2001-03-20) with MANY_PORTS SHARE_IRQ
SERIAL_PCI enabled

> Define "go kablooey". We haven't noticed any problems, and we supplied
> this bit of code.

"Go kablooey" means that all serial output stops and the kernel never
finishes booting.  This patch makes it correctly detect the
controller, continue to produce serial output, and finish booting.

> The size_fifo() routine supplies its own baud rate divisor, and
> on any rs_open() change_speed() sets the baud rate properly.
> I can't figure what you might be seeing.

I don't know why it doesn't work the way you describe.  If I comment
out the section of code that sets the baud rate to 0, everything works
fine.  Otherwise, it doesn't even finish booting.  The Exar datasheet
at http://www.exar.com/products/st16c654.pdf doesn't define what
happens if you set the baud rate registers to 0.

> This isn't necessary. The revision field is only checked
> for 950s, so setting it here doesn't harm anything. If the
> current (only) example of checking it is followed as normal
> procedure, the port type will always be checked first, before
> checking the revision, ensuring only valid revisions are
> referenced.

It's just sloppy programming.  There's no benefit from setting an
invalid revision for the Exar and if the usage of state->revision
changes it may introduce a bug.

> Only saving the revision for 850s is probably wrong. It should
> be saved for all the 85x uarts.

The comment above it should also be changed then.  If someone knows
for sure whether the revision should be saved for the XR16C854 then
the comment can be made clearer.  See patch below.

-VAL

--- linux-2.4.5-pre1/drivers/char/serial.c	Thu Apr 19 00:26:34 2001
+++ linux/drivers/char/serial.c	Tue May 15 03:19:08 2001
@@ -3507,7 +3507,7 @@
 				      struct serial_state *state,
 				      unsigned long flags)
 {
-	unsigned char scratch, scratch2, scratch3;
+	unsigned char scratch, scratch2, scratch3, scratch4;
 
 	/*
 	 * First we check to see if it's an Oxford Semiconductor UART.
@@ -3548,20 +3548,33 @@
 	 * then reading back DLL and DLM.  If DLM reads back 0x10,
 	 * then the UART is a XR16C850 and the DLL contains the chip
 	 * revision.  If DLM reads back 0x14, then the UART is a
-	 * XR16C854.
-	 * 
+	 * XR16C854 and the DLL may or may not contain the revision.
 	 */
+
+	/* Save the DLL and DLM */
+
 	serial_outp(info, UART_LCR, UART_LCR_DLAB);
+	scratch3 = serial_inp(info, UART_DLL);
+	scratch4 = serial_inp(info, UART_DLM);
+
 	serial_outp(info, UART_DLL, 0);
 	serial_outp(info, UART_DLM, 0);
-	state->revision = serial_inp(info, UART_DLL);
+	scratch2 = serial_inp(info, UART_DLL);
 	scratch = serial_inp(info, UART_DLM);
 	serial_outp(info, UART_LCR, 0);
+
 	if (scratch == 0x10 || scratch == 0x14) {
+		state->revision = scratch2;
 		state->type = PORT_16850;
 		return;
 	}
 
+	/* Restore the DLL and DLM */
+
+	serial_outp(info, UART_LCR, UART_LCR_DLAB);
+	serial_outp(info, UART_DLL, scratch3);
+	serial_outp(info, UART_DLM, scratch4);
+	serial_outp(info, UART_LCR, 0);
 	/*
 	 * We distinguish between the '654 and the '650 by counting
 	 * how many bytes are in the FIFO.  I'm using this for now,

Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection

[Stuart MacDonald]

From: "Val Henson" <val@nmt.edu>
> Serial driver version 5.05a (2001-03-20) with MANY_PORTS SHARE_IRQ
> SERIAL_PCI enabled

Hmm. I've been looking at 5.05 (from http://serial.sourceforge.net),
I'm getting 2.4.4 and 2.4.5-pre2 to see what's in there.

> "Go kablooey" means that all serial output stops and the kernel never
> finishes booting.  This patch makes it correctly detect the
> controller, continue to produce serial output, and finish booting.

Kernel version? Distribution? Are you using a serial console?

> I don't know why it doesn't work the way you describe.  If I comment
> out the section of code that sets the baud rate to 0, everything works
> fine.  Otherwise, it doesn't even finish booting.  The Exar datasheet
> at http://www.exar.com/products/st16c654.pdf doesn't define what
> happens if you set the baud rate registers to 0.

Yeah, I double checked the 654 and 864 sheets yesterday. No mention,
which is sorta odd for the 864, since they explicitly say to write the 0s
to get the revision. I'd have to assume that it's bad. This bit I copied
from one of our hardware guys, and it worked, so I never looked too
closely at it. My apologies.

> It's just sloppy programming.  There's no benefit from setting an
> invalid revision for the Exar and if the usage of state->revision
> changes it may introduce a bug.

I agree with you. However, the revision is Ted's code, and I've
generally found it easier to get our patches in to the driver if I
do what he's already doing. However, I agree with you.

> The comment above it should also be changed then.  If someone knows
> for sure whether the revision should be saved for the XR16C854 then
> the comment can be made clearer.  See patch below.

The revision should always be saved if it's available. Hmm.
I didn't look carefully yesterday. The DLL always contains the
revision for the 85x family. Why do you think it doesn't?

I think your patch below is good, I'm just mystified by this
kablooey thing.

..Stu

--- linux-2.4.5-pre1/drivers/char/serial.c Thu Apr 19 00:26:34 2001
+++ linux/drivers/char/serial.c Tue May 15 03:19:08 2001
@@ -3507,7 +3507,7 @@
        struct serial_state *state,
        unsigned long flags)
 {
- unsigned char scratch, scratch2, scratch3;
+ unsigned char scratch, scratch2, scratch3, scratch4;

  /*
  * First we check to see if it's an Oxford Semiconductor UART.
@@ -3548,20 +3548,33 @@
  * then reading back DLL and DLM.  If DLM reads back 0x10,
  * then the UART is a XR16C850 and the DLL contains the chip
  * revision.  If DLM reads back 0x14, then the UART is a
- * XR16C854.
- *
+ * XR16C854 and the DLL may or may not contain the revision.
  */
+
+ /* Save the DLL and DLM */
+
  serial_outp(info, UART_LCR, UART_LCR_DLAB);
+ scratch3 = serial_inp(info, UART_DLL);
+ scratch4 = serial_inp(info, UART_DLM);
+
  serial_outp(info, UART_DLL, 0);
  serial_outp(info, UART_DLM, 0);
- state->revision = serial_inp(info, UART_DLL);
+ scratch2 = serial_inp(info, UART_DLL);
  scratch = serial_inp(info, UART_DLM);
  serial_outp(info, UART_LCR, 0);
+
  if (scratch == 0x10 || scratch == 0x14) {
+ state->revision = scratch2;
  state->type = PORT_16850;
  return;
  }

+ /* Restore the DLL and DLM */
+
+ serial_outp(info, UART_LCR, UART_LCR_DLAB);
+ serial_outp(info, UART_DLL, scratch3);
+ serial_outp(info, UART_DLM, scratch4);
+ serial_outp(info, UART_LCR, 0);
  /*
  * We distinguish between the '654 and the '650 by counting
  * how many bytes are in the FIFO.  I'm using this for now,

Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection

[Val Henson]

Anyone know where Ted Tso is?  He hasn't posted in several weeks.
Alan, will you put this in -ac?  This patch fixes a bug in serial.c
that causes a crash on machines with a ST16C654.

On Tue, May 15, 2001 at 09:52:01AM -0400, Stuart MacDonald wrote:

> Kernel version? Distribution? Are you using a serial console?

2.4.5-pre1 (see patch).

> The revision should always be saved if it's available. Hmm.
> I didn't look carefully yesterday. The DLL always contains the
> revision for the 85x family. Why do you think it doesn't?

Because the comment was ambiguous.  I don't have the data sheet for
the 85x family so I just wrote the code according to the comment.
I'll change the comment to make it clear.

> I think your patch below is good, I'm just mystified by this
> kablooey thing.

Thanks.  Corrected version of the patch is below.

-VAL

--- linux-2.4.5-pre1/drivers/char/serial.c	Thu Apr 19 00:26:34 2001
+++ linux/drivers/char/serial.c	Thu May 17 03:12:09 2001
@@ -3507,7 +3507,7 @@
 				      struct serial_state *state,
 				      unsigned long flags)
 {
-	unsigned char scratch, scratch2, scratch3;
+	unsigned char scratch, scratch2, scratch3, scratch4;
 
 	/*
 	 * First we check to see if it's an Oxford Semiconductor UART.
@@ -3548,20 +3548,33 @@
 	 * then reading back DLL and DLM.  If DLM reads back 0x10,
 	 * then the UART is a XR16C850 and the DLL contains the chip
 	 * revision.  If DLM reads back 0x14, then the UART is a
-	 * XR16C854.
-	 * 
+	 * XR16C854 and the DLL contains the chip revision.
 	 */
+
+	/* Save the DLL and DLM */
+
 	serial_outp(info, UART_LCR, UART_LCR_DLAB);
+	scratch3 = serial_inp(info, UART_DLL);
+	scratch4 = serial_inp(info, UART_DLM);
+
 	serial_outp(info, UART_DLL, 0);
 	serial_outp(info, UART_DLM, 0);
-	state->revision = serial_inp(info, UART_DLL);
+	scratch2 = serial_inp(info, UART_DLL);
 	scratch = serial_inp(info, UART_DLM);
 	serial_outp(info, UART_LCR, 0);
+
 	if (scratch == 0x10 || scratch == 0x14) {
+		state->revision = scratch2;
 		state->type = PORT_16850;
 		return;
 	}
 
+	/* Restore the DLL and DLM */
+
+	serial_outp(info, UART_LCR, UART_LCR_DLAB);
+	serial_outp(info, UART_DLL, scratch3);
+	serial_outp(info, UART_DLM, scratch4);
+	serial_outp(info, UART_LCR, 0);
 	/*
 	 * We distinguish between the '654 and the '650 by counting
 	 * how many bytes are in the FIFO.  I'm using this for now,

Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection

[Theodore Tso]

On Wed, May 16, 2001 at 04:12:45PM -0600, Val Henson wrote:
> Anyone know where Ted Tso is?  He hasn't posted in several weeks.
> Alan, will you put this in -ac?  This patch fixes a bug in serial.c
> that causes a crash on machines with a ST16C654.

I'm around, just swamped with a few other things.  Dealing with serial
driver issues is on the top of my to do list....

						- Ted

Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection

[Stuart MacDonald]

From: "Val Henson" <val@nmt.edu>
> Anyone know where Ted Tso is?  He hasn't posted in several weeks.

Haven't heard from him in a while. I've got a patch or two pending
as well, one of which modifies this code to check for 16c2850s.
Usually he just says he's really busy.

> > Kernel version? Distribution? Are you using a serial console?
> 2.4.5-pre1 (see patch).

Are you using the serial console though? That seems to be
implied by your problem, but I just want to check.

> Because the comment was ambiguous.  I don't have the data sheet for
> the 85x family so I just wrote the code according to the comment.
> I'll change the comment to make it clear.

Hm. I didn't really read the comment, since I know what the
code is doing. :-) I can send you an 864 sheet if you'd like;
we most likely have the other 85x sheets around somewhere;
all pdf format.

..Stu

Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection

[Val Henson]

On Thu, May 17, 2001 at 09:49:11AM -0400, Stuart MacDonald wrote:

> Are you using the serial console though? That seems to be
> implied by your problem, but I just want to check.

Yes, I have serial console only on this board.

-VAL