* [PATCH] drivers/char/serial.c bug in ST16C654 detection
@ 2001-05-12 0:27 Val Henson
2001-05-14 19:50 ` Stuart MacDonald
0 siblings, 1 reply; 8+ messages in thread
From: Val Henson @ 2001-05-12 0:27 UTC (permalink / raw)
To: Theodore Tso; +Cc: linux-kernel
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,
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection 2001-05-12 0:27 [PATCH] drivers/char/serial.c bug in ST16C654 detection Val Henson @ 2001-05-14 19:50 ` Stuart MacDonald 2001-05-14 22:20 ` Val Henson 0 siblings, 1 reply; 8+ messages in thread From: Stuart MacDonald @ 2001-05-14 19:50 UTC (permalink / raw) To: Val Henson, Theodore Tso; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection 2001-05-14 19:50 ` Stuart MacDonald @ 2001-05-14 22:20 ` Val Henson 2001-05-15 13:52 ` Stuart MacDonald 0 siblings, 1 reply; 8+ messages in thread From: Val Henson @ 2001-05-14 22:20 UTC (permalink / raw) To: Stuart MacDonald; +Cc: Theodore Tso, linux-kernel 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, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection 2001-05-14 22:20 ` Val Henson @ 2001-05-15 13:52 ` Stuart MacDonald 2001-05-16 22:12 ` Val Henson 0 siblings, 1 reply; 8+ messages in thread From: Stuart MacDonald @ 2001-05-15 13:52 UTC (permalink / raw) To: Val Henson; +Cc: Theodore Tso, linux-kernel 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, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection 2001-05-15 13:52 ` Stuart MacDonald @ 2001-05-16 22:12 ` Val Henson 2001-05-16 23:12 ` Theodore Tso 2001-05-17 13:49 ` Stuart MacDonald 0 siblings, 2 replies; 8+ messages in thread From: Val Henson @ 2001-05-16 22:12 UTC (permalink / raw) To: Stuart MacDonald; +Cc: Theodore Tso, linux-kernel, Alan Cox 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, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection 2001-05-16 22:12 ` Val Henson @ 2001-05-16 23:12 ` Theodore Tso 2001-05-17 13:49 ` Stuart MacDonald 1 sibling, 0 replies; 8+ messages in thread From: Theodore Tso @ 2001-05-16 23:12 UTC (permalink / raw) To: Val Henson; +Cc: Stuart MacDonald, Theodore Tso, linux-kernel, Alan Cox 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection 2001-05-16 22:12 ` Val Henson 2001-05-16 23:12 ` Theodore Tso @ 2001-05-17 13:49 ` Stuart MacDonald 2001-05-17 23:20 ` Val Henson 1 sibling, 1 reply; 8+ messages in thread From: Stuart MacDonald @ 2001-05-17 13:49 UTC (permalink / raw) To: Val Henson; +Cc: Theodore Tso, linux-kernel, Alan Cox 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/char/serial.c bug in ST16C654 detection 2001-05-17 13:49 ` Stuart MacDonald @ 2001-05-17 23:20 ` Val Henson 0 siblings, 0 replies; 8+ messages in thread From: Val Henson @ 2001-05-17 23:20 UTC (permalink / raw) To: Stuart MacDonald; +Cc: Val Henson, Theodore Tso, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2001-05-18 1:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2001-05-12 0:27 [PATCH] drivers/char/serial.c bug in ST16C654 detection Val Henson 2001-05-14 19:50 ` Stuart MacDonald 2001-05-14 22:20 ` Val Henson 2001-05-15 13:52 ` Stuart MacDonald 2001-05-16 22:12 ` Val Henson 2001-05-16 23:12 ` Theodore Tso 2001-05-17 13:49 ` Stuart MacDonald 2001-05-17 23:20 ` Val Henson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).