| Message ID | 1348222976-7241-1-git-send-email-shubhrajyoti@ti.com |
|---|---|
| State | New |
| Headers | show |
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 ?
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. >
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 >
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 ??
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
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
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 ?
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;
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
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;
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(-)