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

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

[Donald]

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

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

[Alan Cox]

On Mon, 21 May 2012 14:19:34 +0800
"Donald" <donald@asix.com.tw> 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.
> 
> 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

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

[Donald]

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

diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
index 5c27f7e..be493b0 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -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

-----Original Message-----
From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk] 
Sent: Monday, May 21, 2012 5:29 PM
To: Donald
Cc: linux-serial@vger.kernel.org
Subject: Re: Questions regarding adding a patch in linux/drivers/char/8250.c

On Mon, 21 May 2012 14:19:34 +0800
"Donald" <donald@asix.com.tw> 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.
> 
> 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

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

[Donald]

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:donald@asix.com.tw] 
Sent: Tuesday, May 29, 2012 4:38 PM
To: 'Alan Cox'
Cc: 'linux-serial@vger.kernel.org'
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.

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

diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c index 5c27f7e..be493b0 100644
--- a/drivers/tty/serial/8250/8250.c
+++ b/drivers/tty/serial/8250/8250.c
@@ -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

-----Original Message-----
From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
Sent: Monday, May 21, 2012 5:29 PM
To: Donald
Cc: linux-serial@vger.kernel.org
Subject: Re: Questions regarding adding a patch in linux/drivers/char/8250.c

On Mon, 21 May 2012 14:19:34 +0800
"Donald" <donald@asix.com.tw> 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.
> 
> 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

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

[Alan Cox]

> +	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;
> +	}
> +

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

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

[Donald]

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:alan@lxorguk.ukuu.org.uk] 
Sent: Tuesday, June 19, 2012 5:18 PM
To: Donald
Cc: linux-serial@vger.kernel.org
Subject: Re: Questions regarding adding a patch in linux/drivers/char/8250.c


> +	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;
> +	}
> +

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

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

[Alan Cox]

On Mon, 21 May 2012 14:19:34 +0800
"Donald" <donald@asix.com.tw> 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

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

[Donald]

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:alan@lxorguk.ukuu.org.uk] 
Sent: Monday, May 21, 2012 5:30 PM
To: Donald
Cc: linux-serial@vger.kernel.org
Subject: Re: Questions regarding adding a patch in linux/drivers/char/8250.c

On Mon, 21 May 2012 14:19:34 +0800
"Donald" <donald@asix.com.tw> 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

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

[Jason Smith]

On Mon, May 21, 2012 at 4:30 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> 
wrote:
> On Mon, 21 May 2012 14:19:34 +0800
> "Donald" <donald@asix.com.tw> 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


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 <alan@lxorguk.ukuu.org.uk>
To:     "Donald" <donald@asix.com.tw>
Cc:     <linux-serial@vger.kernel.org>
Date:   05/21/2012 04:29 AM
Subject:        Re: Questions regarding adding a patch in 
linux/drivers/char/8250.c
Sent by:        linux-serial-owner@vger.kernel.org



On Mon, 21 May 2012 14:19:34 +0800
"Donald" <donald@asix.com.tw> 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


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