Patchwork serial: omap: fix the overrun case

login
register
mail settings
Submitter Datta, Shubhrajyoti
Date Sept. 21, 2012, 10:22 a.m.
Message ID <1348222976-7241-1-git-send-email-shubhrajyoti@ti.com>
Download mbox | patch
Permalink /patch/185678/
State New
Headers show

Comments

Datta, Shubhrajyoti - Sept. 21, 2012, 10:22 a.m.
Overrun also causes an internal flag to be set, which disables further
reception. Before the next frame can
be received, the MPU must:
• Reset the RX FIFO.
• clear the internal flag.

In the uart mode a dummy read is needed. Add the same.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
- functional testing on omap4sdp
- Verified idle and suspend path hits off on beagle.

 drivers/tty/serial/omap-serial.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
Felipe Balbi - Sept. 21, 2012, 11 a.m.
On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
> Overrun also causes an internal flag to be set, which disables further
> reception. Before the next frame can
> be received, the MPU must:
> • Reset the RX FIFO.
> • clear the internal flag.
> 
> In the uart mode a dummy read is needed. Add the same.

Very nice patch but I think commit log can be a bit more verbose.

Please make the problem a little clearer. Why do we even get that
interrupt fired if BRK_ERROR_BITS aren't set ?

> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> - functional testing on omap4sdp
> - Verified idle and suspend path hits off on beagle.
> 
>  drivers/tty/serial/omap-serial.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index a0d4460..bc22a2b 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
>  static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>  {
>  	unsigned int flag;
> +	unsigned char ch = 0;
> +
> +	if (!(lsr & UART_LSR_BRK_ERROR_BITS))
> +		return;
> +
> +	if (likely(lsr & UART_LSR_DR))
> +		ch = serial_in(up, UART_RX);

Maybe add a comment before this condition stating why this character
read is necessary ?
Datta, Shubhrajyoti - Sept. 21, 2012, 11:16 a.m.
On Friday 21 September 2012 04:30 PM, Felipe Balbi wrote:
> On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
>> Overrun also causes an internal flag to be set, which disables further
>> reception. Before the next frame can
>> be received, the MPU must:
>> • Reset the RX FIFO.
>> • clear the internal flag.
>>
>> In the uart mode a dummy read is needed. Add the same.
> Very nice patch but I think commit log can be a bit more verbose.
ok
> Please make the problem a little clearer. Why do we even get that
> interrupt fired if BRK_ERROR_BITS aren't set ?
I did not get this point.

it it is !  BRK_ERROR_BITS I return.

If it is and there is data in the fifo then a read is done.
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>> - functional testing on omap4sdp
>> - Verified idle and suspend path hits off on beagle.
>>
>>  drivers/tty/serial/omap-serial.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index a0d4460..bc22a2b 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
>>  static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>>  {
>>  	unsigned int flag;
>> +	unsigned char ch = 0;
>> +
>> +	if (!(lsr & UART_LSR_BRK_ERROR_BITS))
>> +		return;
>> +
>> +	if (likely(lsr & UART_LSR_DR))
>> +		ch = serial_in(up, UART_RX);
> Maybe add a comment before this condition stating why this character
> read is necessary ?
OK.
>
Poddar, Sourav - Sept. 21, 2012, 11:18 a.m.
Hi,

On Fri, Sep 21, 2012 at 3:52 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> Overrun also causes an internal flag to be set, which disables further
> reception. Before the next frame can
> be received, the MPU must:
> • Reset the RX FIFO.
> • clear the internal flag.
>
> In the uart mode a dummy read is needed. Add the same.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> - functional testing on omap4sdp
> - Verified idle and suspend path hits off on beagle.
>
>  drivers/tty/serial/omap-serial.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index a0d4460..bc22a2b 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
>  static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>  {
>         unsigned int flag;
> +       unsigned char ch = 0;
> +
> +       if (!(lsr & UART_LSR_BRK_ERROR_BITS))
By using this flag, you are trying to take into account not just the
overrun case but also
frame, parity and break condition case as the flag is the OR of all these.

I suppose the commit log should reflect this.
> +               return;
> +
> +       if (likely(lsr & UART_LSR_DR))
> +               ch = serial_in(up, UART_RX);
>
>         up->port.icount.rx++;
>         flag = TTY_NORMAL;
> --
> 1.7.5.4
>
Felipe Balbi - Sept. 21, 2012, 11:35 a.m.
On Fri, Sep 21, 2012 at 04:46:52PM +0530, Shubhrajyoti wrote:
> On Friday 21 September 2012 04:30 PM, Felipe Balbi wrote:
> > On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
> >> Overrun also causes an internal flag to be set, which disables further
> >> reception. Before the next frame can
> >> be received, the MPU must:
> >> • Reset the RX FIFO.
> >> • clear the internal flag.
> >>
> >> In the uart mode a dummy read is needed. Add the same.
> > Very nice patch but I think commit log can be a bit more verbose.
> ok
> > Please make the problem a little clearer. Why do we even get that
> > interrupt fired if BRK_ERROR_BITS aren't set ?
> I did not get this point.
> 
> it it is !  BRK_ERROR_BITS I return.

That's what I mean. rlsi handler is basically taking care of those
bits... So how come we get RLSI IRQ when those bits aren't set ?

Meaning, you shouldn't ever need that check, right ? Ideally, whenever
that handler is called, it's because BRK_ERROR_BITS are set.

Maybe add a WARN_ONCE() kinda thing just to see if that will ever really
happen ??
Poddar, Sourav - Sept. 21, 2012, 11:39 a.m.
Hi Felipe,

On Fri, Sep 21, 2012 at 4:30 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
>> Overrun also causes an internal flag to be set, which disables further
>> reception. Before the next frame can
>> be received, the MPU must:
>> • Reset the RX FIFO.
>> • clear the internal flag.
>>
>> In the uart mode a dummy read is needed. Add the same.
>
> Very nice patch but I think commit log can be a bit more verbose.
>
> Please make the problem a little clearer. Why do we even get that
> interrupt fired if BRK_ERROR_BITS aren't set ?
>
According to LSR registers, there are few other bits like RX_FIFO_E( atleast 1
character in RX_FIFO or TX FIFO Empty), which might be the cause of an
interrupt. ?
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>> - functional testing on omap4sdp
>> - Verified idle and suspend path hits off on beagle.
>>
>>  drivers/tty/serial/omap-serial.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index a0d4460..bc22a2b 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
>>  static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>>  {
>>       unsigned int flag;
>> +     unsigned char ch = 0;
>> +
>> +     if (!(lsr & UART_LSR_BRK_ERROR_BITS))
>> +             return;
>> +
>> +     if (likely(lsr & UART_LSR_DR))
>> +             ch = serial_in(up, UART_RX);
>
> Maybe add a comment before this condition stating why this character
> read is necessary ?
>
> --
> balbi
Shubhrajyoti Datta - Sept. 21, 2012, 11:48 a.m.
On Fri, Sep 21, 2012 at 5:05 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Fri, Sep 21, 2012 at 04:46:52PM +0530, Shubhrajyoti wrote:
>> On Friday 21 September 2012 04:30 PM, Felipe Balbi wrote:
>> > On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
[...]

>> it it is !  BRK_ERROR_BITS I return.
>
> That's what I mean. rlsi handler is basically taking care of those
> bits... So how come we get RLSI IRQ when those bits aren't set ?
>
> Meaning, you shouldn't ever need that check, right ? Ideally, whenever
> that handler is called, it's because BRK_ERROR_BITS are set.
>
> Maybe add a WARN_ONCE() kinda thing just to see if that will ever really
> happen ??

hmm yes. will get back.

>
> --
> balbi
Felipe Balbi - Sept. 21, 2012, 12:07 p.m.
On Fri, Sep 21, 2012 at 05:09:56PM +0530, Poddar, Sourav wrote:
> Hi Felipe,
> 
> On Fri, Sep 21, 2012 at 4:30 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Fri, Sep 21, 2012 at 03:52:56PM +0530, Shubhrajyoti D wrote:
> >> Overrun also causes an internal flag to be set, which disables further
> >> reception. Before the next frame can
> >> be received, the MPU must:
> >> • Reset the RX FIFO.
> >> • clear the internal flag.
> >>
> >> In the uart mode a dummy read is needed. Add the same.
> >
> > Very nice patch but I think commit log can be a bit more verbose.
> >
> > Please make the problem a little clearer. Why do we even get that
> > interrupt fired if BRK_ERROR_BITS aren't set ?
> >
> According to LSR registers, there are few other bits like RX_FIFO_E( atleast 1
> character in RX_FIFO or TX FIFO Empty), which might be the cause of an
> interrupt. ?

right. In that case, is it really correct to just return if
BRK_ERROR_BITS aren't set ? IRQ line will not toggle and we just might
end up with an IRQ storm, right ?
Kevin Hilman - Sept. 21, 2012, 2:18 p.m.
Shubhrajyoti D <shubhrajyoti@ti.com> writes:

> Overrun also causes an internal flag to be set, which disables further
> reception. Before the next frame can
> be received, the MPU must:
> • Reset the RX FIFO.
> • clear the internal flag.
>
> In the uart mode a dummy read is needed. Add the same.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> - functional testing on omap4sdp
> - Verified idle and suspend path hits off on beagle.

Tested-by: Kevin Hilman <khilman@ti.com>

This fixes the console hang I was seeing on 3530/Overo.

Thanks,

Kevin

>  drivers/tty/serial/omap-serial.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index a0d4460..bc22a2b 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
>  static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>  {
>  	unsigned int flag;
> +	unsigned char ch = 0;
> +
> +	if (!(lsr & UART_LSR_BRK_ERROR_BITS))
> +		return;
> +
> +	if (likely(lsr & UART_LSR_DR))
> +		ch = serial_in(up, UART_RX);
>  
>  	up->port.icount.rx++;
>  	flag = TTY_NORMAL;
Shubhrajyoti Datta - Sept. 21, 2012, 3:08 p.m.
On Fri, Sep 21, 2012 at 7:48 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Shubhrajyoti D <shubhrajyoti@ti.com> writes:
>
[...]
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>> - functional testing on omap4sdp
>> - Verified idle and suspend path hits off on beagle.
>
> Tested-by: Kevin Hilman <khilman@ti.com>
>
> This fixes the console hang I was seeing on 3530/Overo.
Thanks for the test.

Could you test the v2
http://www.spinics.net/lists/arm-kernel/msg197050.html

I have removed the redundant check.
Thanks,


>
> Thanks,
>
> Kevin
>
>>  drivers/tty/serial/omap-serial.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index a0d4460..bc22a2b 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -334,6 +334,13 @@ static unsigned int check_modem_status(struct uart_omap_port *up)
>>  static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>>  {
>>       unsigned int flag;
>> +     unsigned char ch = 0;
>> +
>> +     if (!(lsr & UART_LSR_BRK_ERROR_BITS))
>> +             return;
>> +
>> +     if (likely(lsr & UART_LSR_DR))
>> +             ch = serial_in(up, UART_RX);
>>
>>       up->port.icount.rx++;
>>       flag = TTY_NORMAL;
> --
> 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

Patch

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index a0d4460..bc22a2b 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -334,6 +334,13 @@  static unsigned int check_modem_status(struct uart_omap_port *up)
 static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
 {
 	unsigned int flag;
+	unsigned char ch = 0;
+
+	if (!(lsr & UART_LSR_BRK_ERROR_BITS))
+		return;
+
+	if (likely(lsr & UART_LSR_DR))
+		ch = serial_in(up, UART_RX);
 
 	up->port.icount.rx++;
 	flag = TTY_NORMAL;