Questions regarding adding a patch in linux/drivers/char/8250.c

DISCUSSION Donald 3 participants 9 messages May 21, 2012 – Jun 19, 2012 TTY layer and serial drivers

Share & tools

Donald

author

2012-05-21 06:52 UTC #0 lore raw

Hi,

This is Donald from ASIX Electronics Corp. My company has three PCI to Serial controllers, including MCS9845, MCS9835, and MCS9820. Currently those serial devices using these three chips can directly use the Linux kernel's serial driver in linux/drivers/char/8250.c. Recently we find these three chips have a hardware bug relating to parity error count function. We have a software workaround for this issue. Below for reference is a pseudo code for this workaround.

serial8250_do_set_termios() {
	If ((PID == MCS9845 || PID == MCS935 || PID == MCS9820) && ((termios->c_cflag & PARENB))) {
		port->fifosize = 1; /* Change RX FIFO size to 1 byte */
		up->ier &= ~UART_IER_RLSI; /* Disable RLSI interrupt */
	}	
}

Is it possible to add a patch into linux/drivers/char/8250.c for our chips' hardware issue? If it's not feasible to add a patch into linux/drivers/char/8250.c, could we submit a new driver (e.g., mcs78xx_8250.c) to Linux kernel for devices using these three chips (i.e., those devices can use new driver mcs78xx_8250.c instead of 8250.c)

Your reply to my questions will be very appreciated.

Regards,
Donald Lee
~~~~~~~~~~~~~~~~~~~~~~~~
R&D Division
ASIX Electronics Corporation
TEL: 886-3-5799500 ext.515
http://asix.com.tw/
~~~~~~~~~~~~~~~~~~~~~~~~

Alan Cox

2012-05-21 09:25 UTC #1 lore raw

Quoted 7 lines · On Mon, 21 May 2012 14:19:34 +0800 "Donald" <[email protected]> wrote:

Hi,
This is Donald from ASIX Electronics Corp. My company has three PCI to Serial controllers, including MCS9845, MCS9835, and MCS9820. Currently those serial devices using these three chips can directly use the Linux kernel's serial driver in linux/drivers/char/8250.c. Recently we find these three chips have a hardware bug relating to parity error count function. We have a software workaround for this issue. Below for reference is a pseudo code for this workaround.

Quoted 8 lines

serial8250_do_set_termios() {
	If ((PID == MCS9845 || PID == MCS935 || PID == MCS9820) && ((termios->c_cflag & PARENB))) {
		port->fifosize = 1; /* Change RX FIFO size to 1 byte */
		up->ier &= ~UART_IER_RLSI; /* Disable RLSI interrupt */
	}	
}

Is it possible to add a patch into linux/drivers/char/8250.c for our chips' hardware issue?

Sure.

The only question I would have is how do we reliably detect the presence of the UART devices with this erratum. Can we do it from the PCI identifier ?

Alan

Alan Cox

2012-05-21 09:27 UTC #6 lore raw

In reply to Donald

Quoted 6 lines · On Mon, 21 May 2012 14:19:34 +0800 "Donald" <[email protected]> wrote:

Hi,
This is Donald from ASIX Electronics Corp. My company has three PCI to Serial controllers, including MCS9845, MCS9835, and MCS9820. Currently those serial devices using these three chips can directly use the Linux kernel's serial driver in linux/drivers/char/8250.c. Recently we find these three chips have a hardware bug relating to parity error count function. We have a software workaround for this issue. Below for reference is a pseudo code for this workaround.

Actually I have a second question.

Would it not be better to set the device to 8N1 and do the parity in software rather than lose the FIFO, especially at higher speeds ? Can you clarify what the erratum is triggered by ?

Alan

Donald

author

2012-05-21 12:56 UTC #7 lore raw

Hi Alan,

Thank you for your quick reply and nice questions. We will have an internal discussion regarding your questions and then I will reply to your questions as soon as possible.

Regards,
Donald

-----Original Message-----
From: Alan Cox [mailto:[email protected]]
Sent: Monday, May 21, 2012 5:30 PM
To: Donald
Cc: [email protected]
Subject: Re: Questions regarding adding a patch in linux/drivers/char/8250.c

Quoted 3 lines · On Mon, 21 May 2012 14:19:34 +0800 "Donald" <[email protected]> wrote:

Hi,
This is Donald from ASIX Electronics Corp. My company has three PCI to Serial controllers, including MCS9845, MCS9835, and

MCS9820.

Quoted 3 lines

Currently those serial devices using these three chips can directly
use the Linux kernel's serial driver in linux/drivers/char/8250.c.
Recently we find these three chips have a hardware bug relating to parity error count function. We have a software workaround for

this issue. Below for reference is a pseudo code for this workaround.

Actually I have a second question.

Would it not be better to set the device to 8N1 and do the parity in software rather than lose the FIFO, especially at higher speeds ? Can you clarify what the erratum is triggered by ?

Alan

Jason Smith

2012-05-21 15:07 UTC #8 lore raw

In reply to Alan Cox

Quoted 6 lines · Alan Cox

Quoted 3 lines · On Mon, 21 May 2012 14:19:34 +0800 "Donald" <[email protected]> wrote:
Hi,
This is Donald from ASIX Electronics Corp. My company has three PCI to

Serial controllers, including MCS9845, MCS9835, and MCS9820.

Quoted 1 line

Quoted 1 line
Currently those serial devices using these three chips can directly use

the Linux kernel's serial driver in

Quoted 1 line

Quoted 1 line
linux/drivers/char/8250.c. Recently we find these three chips have a

hardware bug relating to parity error count function. We have a

Quoted 1 line

Quoted 1 line
software workaround for this issue. Below for reference is a pseudo

code for this workaround.

Quoted 5 lines

Actually I have a second question.
Would it not be better to set the device to 8N1 and do the parity in software rather than lose the FIFO, especially at higher speeds ? Can

you

Quoted 3 lines

clarify what the erratum is triggered by ?
Alan

I saw your suggestion and thought it could work for a UART we are working with as well, which cannot identify which bytes have parity errors. However, It seems that doing parity in software would be a fine solution if you are using less than 8 data bits, but would not work with 8 bits + parity. Unless the UART supports 9 data bits, you would be unable to read the parity byte from the UART, and would still have the same problem.

-Jason

From:   Alan Cox <[email protected]>
To:     "Donald" <[email protected]>

Cc <[email protected]>

Date:   05/21/2012 04:29 AM
Subject:        Re: Questions regarding adding a patch in

linux/drivers/char/8250.c
Sent by: [email protected]

Quoted 3 lines · On Mon, 21 May 2012 14:19:34 +0800 "Donald" <[email protected]> wrote:

Hi,
This is Donald from ASIX Electronics Corp. My company has three PCI to

Serial controllers, including MCS9845, MCS9835, and MCS9820.

Quoted 1 line

Currently those serial devices using these three chips can directly use

the Linux kernel's serial driver in

Quoted 1 line

linux/drivers/char/8250.c. Recently we find these three chips have a

hardware bug relating to parity error count function. We have a

Quoted 1 line

software workaround for this issue. Below for reference is a pseudo code

for this workaround.

Actually I have a second question.

Would it not be better to set the device to 8N1 and do the parity in software rather than lose the FIFO, especially at higher speeds ? Can you clarify what the erratum is triggered by ?

Alan

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Donald

author

2012-05-29 08:38 UTC #2 lore raw

In reply to Alan Cox

Hi Alan,

Sorry for my late reply. Below are my answers for your questions in previous emails:

1. Regarding how to detect the presence of the UART devices with this erratum, it's feasible to check the device's vendor identification, which can be acquired by using " container_of" macro.

2. Regarding doing parity check by software, it's not feasible mainly because that the parity bit will be stripped off by the hardware, the software can only get the data portion; hence no chance to do parity check.

3. Regarding the performance concern of the proposed software workaround, we had been verified this workaround and didn't see any performance issue. For example, in a file transfer test at 115200 bps, the performance and CPU utilization rate are almost the same between test drivers with and without this workaround.

4. Below is the complete patch for your preview, upon your approve, I will submit the formal patch by another email. Thank you for your help.

drivers/tty/serial/8250/8250.c | 10 ++++++++++

 1 files changed, 10 insertions(+), 0 deletions(-)

drivers/tty/serial/8250/8250.c +10 -0

index 5c27f7e..be493b0 100644
@@ -38,6 +38,7 @@
 #include <linux/nmi.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pci.h>
 #ifdef CONFIG_SPARC
 #include <linux/sunserialcore.h>
 #endif

@@ -2238,6 +2239,8 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	unsigned long flags;
 	unsigned int baud, quot;
 
+	struct pci_dev *pdev = container_of(port->dev, struct pci_dev, dev);
+
 	switch (termios->c_cflag & CSIZE) {
 	case CS5:
 		cval = UART_LCR_WLEN5;

@@ -2351,6 +2354,13 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (up->capabilities & UART_CAP_RTOIE)
 		up->ier |= UART_IER_RTOIE;
 
+	if ((termios->c_cflag & PARENB) && (pdev->vendor == 0x9710)) {
+		fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_1;
+		up->ier &= ~UART_IER_RLSI;
+	} else {
+		up->ier |= UART_IER_RLSI;
+	}
+
 	serial_port_out(port, UART_IER, up->ier);
 
 	if (up->capabilities & UART_CAP_EFR) {

Signature · 4 lines

1.7.7.6

Regards,
Donald

-----Original Message-----
From: Alan Cox [mailto:[email protected]]
Sent: Monday, May 21, 2012 5:29 PM
To: Donald
Cc: [email protected]
Subject: Re: Questions regarding adding a patch in linux/drivers/char/8250.c

Quoted 3 lines · On Mon, 21 May 2012 14:19:34 +0800 "Donald" <[email protected]> wrote:

Hi,
This is Donald from ASIX Electronics Corp. My company has three PCI to Serial controllers, including MCS9845, MCS9835, and

MCS9820.

Quoted 3 lines

Currently those serial devices using these three chips can directly
use the Linux kernel's serial driver in linux/drivers/char/8250.c.
Recently we find these three chips have a hardware bug relating to parity error count function. We have a software workaround for

this issue. Below for reference is a pseudo code for this workaround.

Quoted 9 lines

serial8250_do_set_termios() {
	If ((PID == MCS9845 || PID == MCS935 || PID == MCS9820) && ((termios->c_cflag & PARENB))) {
		port->fifosize = 1; /* Change RX FIFO size to 1 byte */
		up->ier &= ~UART_IER_RLSI; /* Disable RLSI interrupt */
	}	
}

Is it possible to add a patch into linux/drivers/char/8250.c for our chips' hardware issue?

Sure.

The only question I would have is how do we reliably detect the presence of the UART devices with this erratum. Can we do it from the PCI identifier ?

Alan

Donald

author

2012-06-18 03:08 UTC #3 lore raw

In reply to Alan Cox

Hi Alan,

It's been about three weeks since my last email. It's highly appreciated if you could keep me posted for any update regarding the patch request.

Regards,
Donald

-----Original Message-----
From: Donald [mailto:[email protected]]
Sent: Tuesday, May 29, 2012 4:38 PM
To: 'Alan Cox'
Cc: '[email protected]'
Subject: RE: Questions regarding adding a patch in linux/drivers/char/8250.c

Hi Alan,

Sorry for my late reply. Below are my answers for your questions in previous emails:

1. Regarding how to detect the presence of the UART devices with this erratum, it's feasible to check the device's vendor identification, which can be acquired by using " container_of" macro.

4. Below is the complete patch for your preview, upon your approve, I will submit the formal patch by another email. Thank you for your help.

drivers/tty/serial/8250/8250.c | 10 ++++++++++

 1 files changed, 10 insertions(+), 0 deletions(-)

drivers/tty/serial/8250/8250.c +10 -0

@@ -38,6 +38,7 @@
 #include <linux/nmi.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/pci.h>
 #ifdef CONFIG_SPARC
 #include <linux/sunserialcore.h>
 #endif

@@ -2238,6 +2239,8 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	unsigned long flags;
 	unsigned int baud, quot;
 
+	struct pci_dev *pdev = container_of(port->dev, struct pci_dev, dev);
+
 	switch (termios->c_cflag & CSIZE) {
 	case CS5:
 		cval = UART_LCR_WLEN5;

@@ -2351,6 +2354,13 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (up->capabilities & UART_CAP_RTOIE)
 		up->ier |= UART_IER_RTOIE;
 
+	if ((termios->c_cflag & PARENB) && (pdev->vendor == 0x9710)) {
+		fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_1;
+		up->ier &= ~UART_IER_RLSI;
+	} else {
+		up->ier |= UART_IER_RLSI;
+	}
+
 	serial_port_out(port, UART_IER, up->ier);
 
 	if (up->capabilities & UART_CAP_EFR) {

--
1.7.7.6

Regards,
Donald

Quoted 3 lines · On Mon, 21 May 2012 14:19:34 +0800 "Donald" <[email protected]> wrote:

Hi,
This is Donald from ASIX Electronics Corp. My company has three PCI to Serial controllers, including MCS9845, MCS9835, and

MCS9820.

Quoted 3 lines

Currently those serial devices using these three chips can directly
use the Linux kernel's serial driver in linux/drivers/char/8250.c.
Recently we find these three chips have a hardware bug relating to parity error count function. We have a software workaround for

this issue. Below for reference is a pseudo code for this workaround.

Quoted 9 lines

serial8250_do_set_termios() {
	If ((PID == MCS9845 || PID == MCS935 || PID == MCS9820) && ((termios->c_cflag & PARENB))) {
		port->fifosize = 1; /* Change RX FIFO size to 1 byte */
		up->ier &= ~UART_IER_RLSI; /* Disable RLSI interrupt */
	}	
}

Is it possible to add a patch into linux/drivers/char/8250.c for our chips' hardware issue?

Sure.

The only question I would have is how do we reliably detect the presence of the UART devices with this erratum. Can we do it from the PCI identifier ?

Alan

Alan Cox

2012-06-19 09:14 UTC #4 lore raw

Quoted 7 lines

drivers/tty/serial/8250/8250.c +2 -0

+	struct pci_dev *pdev = container_of(port->dev, struct pci_dev, dev);
+
 	switch (termios->c_cflag & CSIZE) {
 	case CS5:
 		cval = UART_LCR_WLEN5;

drivers/tty/serial/8250/8250.c serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, · line 2363

Quoted 10 lines

drivers/tty/serial/8250/8250.c +7 -0

@@ -2351,6 +2354,13 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (up->capabilities & UART_CAP_RTOIE)
 		up->ier |= UART_IER_RTOIE;
 
+	if ((termios->c_cflag & PARENB) && (pdev->vendor == 0x9710)) {
+		fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_1;
+		up->ier &= ~UART_IER_RLSI;
+	} else {
+		up->ier |= UART_IER_RLSI;
+	}
+

It's on my TODO list for the 3.6 merge. Your patch doesn't work (in fact it crashes in some cases) because it blindly assumes an 8250 port is on the PCI bus.

However it documents everything I need to know to push an actual fix.

Alan

Donald

author

2012-06-19 12:36 UTC #5 lore raw

Hi Alan,

Thank you for your nice update. I will try to reproduce the crash case on my end and do further analysis. Any update I will keep you posted. Thanks.

Donald

-----Original Message-----
From: Alan Cox [mailto:[email protected]]
Sent: Tuesday, June 19, 2012 5:18 PM
To: Donald
Cc: [email protected]
Subject: Re: Questions regarding adding a patch in linux/drivers/char/8250.c

Quoted 6 lines

drivers/tty/serial/8250/8250.c +2 -0

+	struct pci_dev *pdev = container_of(port->dev, struct pci_dev, dev);
+
 	switch (termios->c_cflag & CSIZE) {
 	case CS5:
 		cval = UART_LCR_WLEN5;

drivers/tty/serial/8250/8250.c serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, · line 2363

Quoted 10 lines

drivers/tty/serial/8250/8250.c +7 -0

@@ -2351,6 +2354,13 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (up->capabilities & UART_CAP_RTOIE)
 		up->ier |= UART_IER_RTOIE;
 
+	if ((termios->c_cflag & PARENB) && (pdev->vendor == 0x9710)) {
+		fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_1;
+		up->ier &= ~UART_IER_RLSI;
+	} else {
+		up->ier |= UART_IER_RLSI;
+	}
+

It's on my TODO list for the 3.6 merge. Your patch doesn't work (in fact it crashes in some cases) because it blindly assumes an 8250 port is on the PCI bus.

However it documents everything I need to know to push an actual fix.

Alan