[PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag

[PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag

[Vikram Pandita]

OMAP3630 and OMAP4430 UART IP blocks have a restriction wrt RX FIFO.
Empty RX fifo read causes an abort. OMAP1/2/3 do not have this restriction.

So interoduce a flag in 8250 driver: UPF_NO_EMPTY_FIFO_READ

If this flag is specified for an 8250 port, then first check if rx fifo has
contents, and only then proceed to read the fifo.

The change affects only boards passing this flag in port data and hence is
non-disruptive for other boards.

First reported/fixed on omap4430 by Shilimkar, Santosh

Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
Cc: Shilimkar, Santosh <santosh.shilimkar@ti.com>
---
 drivers/serial/8250.c       |   30 +++++++++++++++++++++++++-----
 include/linux/serial_core.h |    1 +
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 737b4c9..03c890e 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1245,7 +1245,11 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 #endif
 	serial_outp(up, UART_MCR, save_mcr);
 	serial8250_clear_fifos(up);
-	serial_in(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			serial_in(up, UART_RX);
+	} else
+		serial_in(up, UART_RX);
 	if (up->capabilities & UART_CAP_UUE)
 		serial_outp(up, UART_IER, UART_IER_UUE);
 	else
@@ -1289,7 +1293,11 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	}
 	serial_outp(up, UART_IER, 0x0f);	/* enable all intrs */
 	(void)serial_inp(up, UART_LSR);
-	(void)serial_inp(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			(void)serial_inp(up, UART_RX);
+	} else
+		(void)serial_inp(up, UART_RX);
 	(void)serial_inp(up, UART_IIR);
 	(void)serial_inp(up, UART_MSR);
 	serial_outp(up, UART_TX, 0xFF);
@@ -1984,7 +1992,11 @@ static int serial8250_startup(struct uart_port *port)
 	 * Clear the interrupt registers.
 	 */
 	(void) serial_inp(up, UART_LSR);
-	(void) serial_inp(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			(void) serial_inp(up, UART_RX);
+	} else
+		(void) serial_inp(up, UART_RX);
 	(void) serial_inp(up, UART_IIR);
 	(void) serial_inp(up, UART_MSR);
 
@@ -2141,7 +2153,11 @@ dont_test_tx_en:
 	 * routines or the previous session.
 	 */
 	serial_inp(up, UART_LSR);
-	serial_inp(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			serial_inp(up, UART_RX);
+	} else
+		serial_inp(up, UART_RX);
 	serial_inp(up, UART_IIR);
 	serial_inp(up, UART_MSR);
 	up->lsr_saved_flags = 0;
@@ -2207,7 +2223,11 @@ static void serial8250_shutdown(struct uart_port *port)
 	 * Read data port to reset things, and then unlink from
 	 * the IRQ chain.
 	 */
-	(void) serial_in(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			(void) serial_inp(up, UART_RX);
+	} else
+		(void) serial_in(up, UART_RX);
 
 	del_timer_sync(&up->timer);
 	up->timer.function = serial8250_timeout;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index db532ce..0d65bb3 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -308,6 +308,7 @@ struct uart_port {
 #define UPF_SPD_WARP		((__force upf_t) (0x1010))
 #define UPF_SKIP_TEST		((__force upf_t) (1 << 6))
 #define UPF_AUTO_IRQ		((__force upf_t) (1 << 7))
+#define UPF_NO_EMPTY_FIFO_READ	((__force upf_t) (1 << 8))
 #define UPF_HARDPPS_CD		((__force upf_t) (1 << 11))
 #define UPF_LOW_LATENCY		((__force upf_t) (1 << 13))
 #define UPF_BUGGY_UART		((__force upf_t) (1 << 14))
-- 
1.6.5.1.69.g36942

Re: [PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag

[Tony Lindgren]

* Vikram Pandita <vikram.pandita@ti.com> [091116 15:00]:
> OMAP3630 and OMAP4430 UART IP blocks have a restriction wrt RX FIFO.
> Empty RX fifo read causes an abort. OMAP1/2/3 do not have this restriction.
> 
> So interoduce a flag in 8250 driver: UPF_NO_EMPTY_FIFO_READ
> 
> If this flag is specified for an 8250 port, then first check if rx fifo has
> contents, and only then proceed to read the fifo.
> 
> The change affects only boards passing this flag in port data and hence is
> non-disruptive for other boards.
> 
> First reported/fixed on omap4430 by Shilimkar, Santosh

This should go to linux-serial@vger.kernel.org and Alan Cox.

Regards,

Tony
 
> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
> Cc: Shilimkar, Santosh <santosh.shilimkar@ti.com>
> ---
>  drivers/serial/8250.c       |   30 +++++++++++++++++++++++++-----
>  include/linux/serial_core.h |    1 +
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 737b4c9..03c890e 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1245,7 +1245,11 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
>  #endif
>  	serial_outp(up, UART_MCR, save_mcr);
>  	serial8250_clear_fifos(up);
> -	serial_in(up, UART_RX);
> +	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
> +		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
> +			serial_in(up, UART_RX);
> +	} else
> +		serial_in(up, UART_RX);
>  	if (up->capabilities & UART_CAP_UUE)
>  		serial_outp(up, UART_IER, UART_IER_UUE);
>  	else
> @@ -1289,7 +1293,11 @@ static void autoconfig_irq(struct uart_8250_port *up)
>  	}
>  	serial_outp(up, UART_IER, 0x0f);	/* enable all intrs */
>  	(void)serial_inp(up, UART_LSR);
> -	(void)serial_inp(up, UART_RX);
> +	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
> +		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
> +			(void)serial_inp(up, UART_RX);
> +	} else
> +		(void)serial_inp(up, UART_RX);
>  	(void)serial_inp(up, UART_IIR);
>  	(void)serial_inp(up, UART_MSR);
>  	serial_outp(up, UART_TX, 0xFF);
> @@ -1984,7 +1992,11 @@ static int serial8250_startup(struct uart_port *port)
>  	 * Clear the interrupt registers.
>  	 */
>  	(void) serial_inp(up, UART_LSR);
> -	(void) serial_inp(up, UART_RX);
> +	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
> +		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
> +			(void) serial_inp(up, UART_RX);
> +	} else
> +		(void) serial_inp(up, UART_RX);
>  	(void) serial_inp(up, UART_IIR);
>  	(void) serial_inp(up, UART_MSR);
>  
> @@ -2141,7 +2153,11 @@ dont_test_tx_en:
>  	 * routines or the previous session.
>  	 */
>  	serial_inp(up, UART_LSR);
> -	serial_inp(up, UART_RX);
> +	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
> +		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
> +			serial_inp(up, UART_RX);
> +	} else
> +		serial_inp(up, UART_RX);
>  	serial_inp(up, UART_IIR);
>  	serial_inp(up, UART_MSR);
>  	up->lsr_saved_flags = 0;
> @@ -2207,7 +2223,11 @@ static void serial8250_shutdown(struct uart_port *port)
>  	 * Read data port to reset things, and then unlink from
>  	 * the IRQ chain.
>  	 */
> -	(void) serial_in(up, UART_RX);
> +	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
> +		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
> +			(void) serial_inp(up, UART_RX);
> +	} else
> +		(void) serial_in(up, UART_RX);
>  
>  	del_timer_sync(&up->timer);
>  	up->timer.function = serial8250_timeout;
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index db532ce..0d65bb3 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -308,6 +308,7 @@ struct uart_port {
>  #define UPF_SPD_WARP		((__force upf_t) (0x1010))
>  #define UPF_SKIP_TEST		((__force upf_t) (1 << 6))
>  #define UPF_AUTO_IRQ		((__force upf_t) (1 << 7))
> +#define UPF_NO_EMPTY_FIFO_READ	((__force upf_t) (1 << 8))
>  #define UPF_HARDPPS_CD		((__force upf_t) (1 << 11))
>  #define UPF_LOW_LATENCY		((__force upf_t) (1 << 13))
>  #define UPF_BUGGY_UART		((__force upf_t) (1 << 14))
> -- 
> 1.6.5.1.69.g36942
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag

[Pandita, Vikram]

Tony

>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Tony
>Lindgren
>Sent: Tuesday, November 17, 2009 11:06 AM
>To: Pandita, Vikram
>Cc: linux-omap@vger.kernel.org; Shilimkar, Santosh
>Subject: Re: [PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag
>
>* Vikram Pandita <vikram.pandita@ti.com> [091116 15:00]:
>> OMAP3630 and OMAP4430 UART IP blocks have a restriction wrt RX FIFO.
>> Empty RX fifo read causes an abort. OMAP1/2/3 do not have this restriction.
>>
>> So interoduce a flag in 8250 driver: UPF_NO_EMPTY_FIFO_READ
>>
>> If this flag is specified for an 8250 port, then first check if rx fifo has
>> contents, and only then proceed to read the fifo.
>>
>> The change affects only boards passing this flag in port data and hence is
>> non-disruptive for other boards.
>>
>> First reported/fixed on omap4430 by Shilimkar, Santosh
>
>This should go to linux-serial@vger.kernel.org and Alan Cox.

Patches were posted to l-o for review. Next step was to get it to the right subsystem.

I thought Alan Cox stepped down from the maintainership of serial/tty subsystem.
Not sure if Andrew Morton is taking these patches to MM tree for now.
Any idea?

>
>Regards,
>
>Tony
<snip>

Re: [PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag

[Tony Lindgren]

* Pandita, Vikram <vikram.pandita@ti.com> [091117 09:39]:
> Tony
> 
> >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of Tony
> >Lindgren
> >Sent: Tuesday, November 17, 2009 11:06 AM
> >To: Pandita, Vikram
> >Cc: linux-omap@vger.kernel.org; Shilimkar, Santosh
> >Subject: Re: [PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag
> >
> >* Vikram Pandita <vikram.pandita@ti.com> [091116 15:00]:
> >> OMAP3630 and OMAP4430 UART IP blocks have a restriction wrt RX FIFO.
> >> Empty RX fifo read causes an abort. OMAP1/2/3 do not have this restriction.
> >>
> >> So interoduce a flag in 8250 driver: UPF_NO_EMPTY_FIFO_READ
> >>
> >> If this flag is specified for an 8250 port, then first check if rx fifo has
> >> contents, and only then proceed to read the fifo.
> >>
> >> The change affects only boards passing this flag in port data and hence is
> >> non-disruptive for other boards.
> >>
> >> First reported/fixed on omap4430 by Shilimkar, Santosh
> >
> >This should go to linux-serial@vger.kernel.org and Alan Cox.
> 
> Patches were posted to l-o for review. Next step was to get it to the right subsystem.
> 
> I thought Alan Cox stepped down from the maintainership of serial/tty subsystem.
> Not sure if Andrew Morton is taking these patches to MM tree for now.
> Any idea?

OK, no idea who's dealing with 8250 now :)

Tony

[PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag

[Vikram Pandita]

OMAP3630 and OMAP4430 UART IP blocks have a restriction wrt RX FIFO.
Empty RX fifo read causes an abort. OMAP1/2/3 do not have this restriction.

So interoduce a flag in 8250 driver: UPF_NO_EMPTY_FIFO_READ

If this flag is specified for an 8250 port, then first check if rx fifo has
contents, and only then proceed to read the fifo.

The change affects only boards passing this flag in port data and hence is
non-disruptive for other boards.

First reported/fixed on omap4430 by Shilimkar, Santosh

Cc: Shilimkar, Santosh <santosh.shilimkar@ti.com>
Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
---
 drivers/serial/8250.c       |   30 +++++++++++++++++++++++++-----
 include/linux/serial_core.h |    1 +
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 737b4c9..03c890e 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1245,7 +1245,11 @@ static void autoconfig(struct uart_8250_port *up, unsigned int probeflags)
 #endif
 	serial_outp(up, UART_MCR, save_mcr);
 	serial8250_clear_fifos(up);
-	serial_in(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			serial_in(up, UART_RX);
+	} else
+		serial_in(up, UART_RX);
 	if (up->capabilities & UART_CAP_UUE)
 		serial_outp(up, UART_IER, UART_IER_UUE);
 	else
@@ -1289,7 +1293,11 @@ static void autoconfig_irq(struct uart_8250_port *up)
 	}
 	serial_outp(up, UART_IER, 0x0f);	/* enable all intrs */
 	(void)serial_inp(up, UART_LSR);
-	(void)serial_inp(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			(void)serial_inp(up, UART_RX);
+	} else
+		(void)serial_inp(up, UART_RX);
 	(void)serial_inp(up, UART_IIR);
 	(void)serial_inp(up, UART_MSR);
 	serial_outp(up, UART_TX, 0xFF);
@@ -1984,7 +1992,11 @@ static int serial8250_startup(struct uart_port *port)
 	 * Clear the interrupt registers.
 	 */
 	(void) serial_inp(up, UART_LSR);
-	(void) serial_inp(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			(void) serial_inp(up, UART_RX);
+	} else
+		(void) serial_inp(up, UART_RX);
 	(void) serial_inp(up, UART_IIR);
 	(void) serial_inp(up, UART_MSR);
 
@@ -2141,7 +2153,11 @@ dont_test_tx_en:
 	 * routines or the previous session.
 	 */
 	serial_inp(up, UART_LSR);
-	serial_inp(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			serial_inp(up, UART_RX);
+	} else
+		serial_inp(up, UART_RX);
 	serial_inp(up, UART_IIR);
 	serial_inp(up, UART_MSR);
 	up->lsr_saved_flags = 0;
@@ -2207,7 +2223,11 @@ static void serial8250_shutdown(struct uart_port *port)
 	 * Read data port to reset things, and then unlink from
 	 * the IRQ chain.
 	 */
-	(void) serial_in(up, UART_RX);
+	if (up->port.flags & UPF_NO_EMPTY_FIFO_READ) {
+		if (serial_inp(up, UART_LSR) & UART_LSR_DR)
+			(void) serial_inp(up, UART_RX);
+	} else
+		(void) serial_in(up, UART_RX);
 
 	del_timer_sync(&up->timer);
 	up->timer.function = serial8250_timeout;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index db532ce..0d65bb3 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -308,6 +308,7 @@ struct uart_port {
 #define UPF_SPD_WARP		((__force upf_t) (0x1010))
 #define UPF_SKIP_TEST		((__force upf_t) (1 << 6))
 #define UPF_AUTO_IRQ		((__force upf_t) (1 << 7))
+#define UPF_NO_EMPTY_FIFO_READ	((__force upf_t) (1 << 8))
 #define UPF_HARDPPS_CD		((__force upf_t) (1 << 11))
 #define UPF_LOW_LATENCY		((__force upf_t) (1 << 13))
 #define UPF_BUGGY_UART		((__force upf_t) (1 << 14))
-- 
1.6.5.1.69.g36942

Re: [PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag

[Alan Cox]

On Tue, 17 Nov 2009 15:16:55 -0600
Vikram Pandita <vikram.pandita@ti.com> wrote:

> OMAP3630 and OMAP4430 UART IP blocks have a restriction wrt RX FIFO.
> Empty RX fifo read causes an abort. OMAP1/2/3 do not have this restriction.

Nothing wrong with the requirement but I think it would keep 8250.c a lot
cleaner if you implemented that in the serial_in method for the
OMAP3630/4430. It would also mean anyone adding a UART_RX or any cases
you miss don't break in future

This is especially true now as you can pass a private
serial_in/serial_out method in the port register function.

Care to submit an alternate patch doing it that way, or see any reason
for not ?

Alan

RE: [PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag

[Pandita, Vikram]

>-----Original Message-----
>From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
>Sent: Tuesday, November 17, 2009 3:26 PM
>To: Pandita, Vikram
>Cc: linux-serial@vger.kernel.org; akpm@linux-foundation.org; linux-omap@vger.kernel.org; Pandita,
>Vikram; Shilimkar, Santosh
>Subject: Re: [PATCH 1/2] serial: 8250: add UPF_NO_EMPTY_FIFO_READ flag
>
>On Tue, 17 Nov 2009 15:16:55 -0600
>Vikram Pandita <vikram.pandita@ti.com> wrote:
>
>> OMAP3630 and OMAP4430 UART IP blocks have a restriction wrt RX FIFO.
>> Empty RX fifo read causes an abort. OMAP1/2/3 do not have this restriction.
>
>Nothing wrong with the requirement but I think it would keep 8250.c a lot
>cleaner if you implemented that in the serial_in method for the
>OMAP3630/4430. It would also mean anyone adding a UART_RX or any cases
>you miss don't break in future

Agree. Was contemplating this change in beginning, but lost that train of thought.
Thx for your review time.

>
>This is especially true now as you can pass a private
>serial_in/serial_out method in the port register function.

Our requirement is to over-right the serial_in for now.
Is it fine to just add serial_in over-load or do you suggest both in/out?

>
>Care to submit an alternate patch doing it that way, or see any reason
>for not ?

Working on it right away. See my patch in an hours time.

>
>Alan