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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort
>-Original Message- >From: Alan Cox [mailto:a...@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 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 > >(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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 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 (Please push it via the OMAP tree as its now OMAP specific) Alan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort
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
Cc: Alan Cox
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] omap: serial: fix non-empty uart fifo read abort
Vikram Pandita 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
> Cc: Alan Cox
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
