[PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort

[PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort

[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.

Overrigt the default 8250 read handler: mem_serial_in()
by a custom handler: serial_in_8250()

serial_in_8250() makes sure that RX fifo is not read when empty,
on omap4 and 3630 silicons only

tested on zoom3(3630) board

Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
v1: initial implementation
	http://patchwork.kernel.org/patch/60785/
	http://patchwork.kernel.org/patch/60786/

v2: incorporate review comments from Alan Cox
	http://patchwork.kernel.org/patch/60785/
	No 8250 driver change required now

 arch/arm/mach-omap2/serial.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 2e17b57..362cb82 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -71,6 +71,30 @@ struct omap_uart_state {
 
 static LIST_HEAD(uart_list);
 
+static struct omap_uart_state omap_uart[];
+static inline unsigned int serial_read_reg(struct plat_serial8250_port *, int);
+
+/*
+ * Overrigt the default 8250 read handler: mem_serial_in()
+ * Empty RX fifo read causes an abort on omap3630 and omap4
+ * This function makes sure that an empty rx fifo is not read on these silicons
+ * (OMAP1/2/3 are not affected)
+ */
+static unsigned int serial_in_8250(struct uart_port *up, int offset)
+{
+	/* Do not read empty UART fifo on omap3630/44xx */
+	if ((UART_RX == offset) &&
+		(cpu_is_omap3630() || cpu_is_omap44xx())) {
+
+		unsigned int lsr;
+
+		lsr = serial_read_reg(omap_uart[up->line].p, UART_LSR);
+		if (!(lsr & UART_LSR_DR))
+			return 0;
+	}
+	return serial_read_reg(omap_uart[up->line].p, offset);
+}
+
 static struct plat_serial8250_port serial_platform_data0[] = {
 	{
 		.mapbase	= OMAP_UART1_BASE,
@@ -79,6 +103,7 @@ static struct plat_serial8250_port serial_platform_data0[] = {
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= OMAP24XX_BASE_BAUD * 16,
+		.serial_in	= serial_in_8250,
 	}, {
 		.flags		= 0
 	}
@@ -92,6 +117,7 @@ static struct plat_serial8250_port serial_platform_data1[] = {
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= OMAP24XX_BASE_BAUD * 16,
+		.serial_in	= serial_in_8250,
 	}, {
 		.flags		= 0
 	}
@@ -105,6 +131,7 @@ static struct plat_serial8250_port serial_platform_data2[] = {
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= OMAP24XX_BASE_BAUD * 16,
+		.serial_in	= serial_in_8250,
 	}, {
 		.flags		= 0
 	}
@@ -119,6 +146,7 @@ static struct plat_serial8250_port serial_platform_data3[] = {
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
 		.uartclk	= OMAP24XX_BASE_BAUD * 16,
+		.serial_in	= serial_in_8250,
 	}, {
 		.flags		= 0
 	}
-- 
1.6.5.1.69.g36942

Re: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort

[Kevin Hilman]

Vikram Pandita <vikram.pandita@ti.com> writes:

> 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.
>
> Overrigt the default 8250 read handler: mem_serial_in()
> by a custom handler: serial_in_8250()
>
> serial_in_8250() makes sure that RX fifo is not read when empty,
> on omap4 and 3630 silicons only
>
> tested on zoom3(3630) board
>
> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
> v1: initial implementation
> 	http://patchwork.kernel.org/patch/60785/
> 	http://patchwork.kernel.org/patch/60786/
>
> v2: incorporate review comments from Alan Cox
> 	http://patchwork.kernel.org/patch/60785/
> 	No 8250 driver change required now
>
>  arch/arm/mach-omap2/serial.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 2e17b57..362cb82 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -71,6 +71,30 @@ struct omap_uart_state {
>  
>  static LIST_HEAD(uart_list);
>  
> +static struct omap_uart_state omap_uart[];
> +static inline unsigned int serial_read_reg(struct plat_serial8250_port *, int);
> +
> +/*
> + * Overrigt the default 8250 read handler: mem_serial_in()
> + * Empty RX fifo read causes an abort on omap3630 and omap4
> + * This function makes sure that an empty rx fifo is not read on these silicons
> + * (OMAP1/2/3 are not affected)
> + */
> +static unsigned int serial_in_8250(struct uart_port *up, int offset)
> +{
> +	/* Do not read empty UART fifo on omap3630/44xx */
> +	if ((UART_RX == offset) &&
> +		(cpu_is_omap3630() || cpu_is_omap44xx())) {

Rather than the cpu_is_* here, why not just override the function 
for these platforms and leave the default for 34xx/35xx.

Kevin

Re: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort

[Nishanth Menon]

Pandita, Vikram had written, on 11/17/2009 05:39 PM, the following:
> 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.
> 
> Overrigt the default 8250 read handler: mem_serial_in()
   ^^^^^^^^ <- I am just being a nuisance, but I think you mean override

> by a custom handler: serial_in_8250()
> 
> serial_in_8250() makes sure that RX fifo is not read when empty,
> on omap4 and 3630 silicons only
> 
> tested on zoom3(3630) board
> 
> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
> v1: initial implementation
> 	http://patchwork.kernel.org/patch/60785/
> 	http://patchwork.kernel.org/patch/60786/
> 
> v2: incorporate review comments from Alan Cox
> 	http://patchwork.kernel.org/patch/60785/
> 	No 8250 driver change required now
> 
>  arch/arm/mach-omap2/serial.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 2e17b57..362cb82 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -71,6 +71,30 @@ struct omap_uart_state {
>  
>  static LIST_HEAD(uart_list);
>  
> +static struct omap_uart_state omap_uart[];
> +static inline unsigned int serial_read_reg(struct plat_serial8250_port *, int);
> +
> +/*
> + * Overrigt the default 8250 read handler: mem_serial_in()
       ^^^^^^^^^ <- same here
> + * Empty RX fifo read causes an abort on omap3630 and omap4
> + * This function makes sure that an empty rx fifo is not read on these silicons
> + * (OMAP1/2/3 are not affected)
> + */
> +static unsigned int serial_in_8250(struct uart_port *up, int offset)
> +{
> +	/* Do not read empty UART fifo on omap3630/44xx */
> +	if ((UART_RX == offset) &&
> +		(cpu_is_omap3630() || cpu_is_omap44xx())) {
Do we want to use FEATURE here? I can expect to see more silicons use 
this new UART IP, so using the OMAP FEATURE framework might make sense.
I wonder if you can at least use MVR_REG to detect if we want to enable 
this?
or better still we could even skip the check.. might save cpu cycles..
> +
> +		unsigned int lsr;
> +
> +		lsr = serial_read_reg(omap_uart[up->line].p, UART_LSR);
> +		if (!(lsr & UART_LSR_DR))
> +			return 0;
> +	}
> +	return serial_read_reg(omap_uart[up->line].p, offset);
> +}
> +
>  static struct plat_serial8250_port serial_platform_data0[] = {
>  	{
>  		.mapbase	= OMAP_UART1_BASE,
> @@ -79,6 +103,7 @@ static struct plat_serial8250_port serial_platform_data0[] = {
>  		.iotype		= UPIO_MEM,
>  		.regshift	= 2,
>  		.uartclk	= OMAP24XX_BASE_BAUD * 16,
> +		.serial_in	= serial_in_8250,
>  	}, {
>  		.flags		= 0
>  	}
> @@ -92,6 +117,7 @@ static struct plat_serial8250_port serial_platform_data1[] = {
>  		.iotype		= UPIO_MEM,
>  		.regshift	= 2,
>  		.uartclk	= OMAP24XX_BASE_BAUD * 16,
> +		.serial_in	= serial_in_8250,
>  	}, {
>  		.flags		= 0
>  	}
> @@ -105,6 +131,7 @@ static struct plat_serial8250_port serial_platform_data2[] = {
>  		.iotype		= UPIO_MEM,
>  		.regshift	= 2,
>  		.uartclk	= OMAP24XX_BASE_BAUD * 16,
> +		.serial_in	= serial_in_8250,
>  	}, {
>  		.flags		= 0
>  	}
> @@ -119,6 +146,7 @@ static struct plat_serial8250_port serial_platform_data3[] = {
>  		.iotype		= UPIO_MEM,
>  		.regshift	= 2,
>  		.uartclk	= OMAP24XX_BASE_BAUD * 16,
> +		.serial_in	= serial_in_8250,
>  	}, {
>  		.flags		= 0
>  	}


-- 
Regards,
Nishanth Menon

Re: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort

[Alan Cox]

On Tue, 17 Nov 2009 17:39:11 -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.
> 
> Overrigt the default 8250 read handler: mem_serial_in()
> by a custom handler: serial_in_8250()
> 
> serial_in_8250() makes sure that RX fifo is not read when empty,
> on omap4 and 3630 silicons only
> 
> tested on zoom3(3630) board

Acked-by: Alan Cox <alan@linux.intel.com>

(Please push it via the OMAP tree as its now OMAP specific)


Alan

RE: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort

[Pandita, Vikram]

>-----Original Message-----
>From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
>Sent: Tuesday, November 17, 2009 7:00 PM
>To: Pandita, Vikram
>Cc: linux-omap@vger.kernel.org; Pandita, Vikram
>Subject: Re: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort
>
>On Tue, 17 Nov 2009 17:39:11 -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.
>>
>> Overrigt the default 8250 read handler: mem_serial_in()
>> by a custom handler: serial_in_8250()
>>
>> serial_in_8250() makes sure that RX fifo is not read when empty,
>> on omap4 and 3630 silicons only
>>
>> tested on zoom3(3630) board
>
>Acked-by: Alan Cox <alan@linux.intel.com>
>
>(Please push it via the OMAP tree as its now OMAP specific)

Thanks for the suggestion that made it 8250 independent.
I am scared to touch this driver :)

Yes I would get this patch upstream via Tony now.
I am getting some minor review comments on linux-omap, will fix and repost.


>
>
>Alan

RE: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort

[Gadiyar, Anand]

On Tue, 17 Nov 2009 17:39:11 -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.

Minor nitpicking for the changelog.

OMAP3430 is better instead of OMAP3 here, since the 3630 also qualifies
as OMAP3

>
> Overrigt the default 8250 read handler: mem_serial_in()

s/Overrigt/Overwrite

> by a custom handler: serial_in_8250()
>
> serial_in_8250() makes sure that RX fifo is not read when empty,
> on omap4 and 3630 silicons only
>
> tested on zoom3(3630) board