2007-05-31 19:34:23

by Haavard Skinnemoen

[permalink] [raw]
Subject: [PATCH] atmel_serial: Fix break handling

The RXBRK field in the AT91/AT32 USART status register has the
following definition according to e.g. the AT32AP7000 data sheet:

RXBRK: Break Received/End of Break
0: No Break received or End of Break detected since the last RSTSTA.
1: Break Received or End of Break detected since the last RSTSTA.

Thus, for each break, the USART sets the RXBRK bit twice. This patch
modifies the driver to report the break event to the serial core only
once by keeping track of whether a break condition is currently
active. The break_active flag is reset as soon as a character is
received, so even if we miss the start-of-break interrupt this should
do the right thing.

Signed-off-by: Haavard Skinnemoen <[email protected]>
---
It's been almost a year since the last patch I sent attempting to fix
this. Sorry for not following up any sooner.

Anyway, here's a new attempt. It should work even if we miss some
interrupts, and it should not break break handling by ignoring the
return value from uart_handle_break() as Russell pointed out.

There may be cases which aren't handled well: If you send two breaks
with no characters in between and we miss an interrupt on the first
one, the driver probably gets a bit confused. I don't see any way to
fix this, but the driver should get un-confused upon reception of the
next character.

I've tested this with Magic SysRq on ATSTK1000: It works with this
patch but not without. Ivan Kuten reported in a different thread that
SysRq didn't work on AT91RM9200 and I very much doubt it works on
AT91SAM926x.

The break count in /proc/tty/driver/atmel_serial also makes more sense
with this patch applied; without it, the break count increments by two
every time I send a break.

drivers/serial/atmel_serial.c | 32 ++++++++++++++++++++++++++++++--
1 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/atmel_serial.c b/drivers/serial/atmel_serial.c
index 3320bcd..4d6b3c5 100644
--- a/drivers/serial/atmel_serial.c
+++ b/drivers/serial/atmel_serial.c
@@ -114,6 +114,7 @@ struct atmel_uart_port {
struct uart_port uart; /* uart */
struct clk *clk; /* uart clock */
unsigned short suspended; /* is port suspended? */
+ int break_active; /* break being received */
};

static struct atmel_uart_port atmel_ports[ATMEL_MAX_UART];
@@ -252,6 +253,7 @@ static void atmel_break_ctl(struct uart_port *port, int break_state)
*/
static void atmel_rx_chars(struct uart_port *port)
{
+ struct atmel_uart_port *atmel_port = (struct atmel_uart_port *) port;
struct tty_struct *tty = port->info->tty;
unsigned int status, ch, flg;

@@ -267,13 +269,29 @@ static void atmel_rx_chars(struct uart_port *port)
* note that the error handling code is
* out of the main execution path
*/
- if (unlikely(status & (ATMEL_US_PARE | ATMEL_US_FRAME | ATMEL_US_OVRE | ATMEL_US_RXBRK))) {
+ if (unlikely(status & (ATMEL_US_PARE | ATMEL_US_FRAME
+ | ATMEL_US_OVRE | ATMEL_US_RXBRK)
+ || atmel_port->break_active)) {
UART_PUT_CR(port, ATMEL_US_RSTSTA); /* clear error */
- if (status & ATMEL_US_RXBRK) {
+ if (status & ATMEL_US_RXBRK
+ && !atmel_port->break_active) {
status &= ~(ATMEL_US_PARE | ATMEL_US_FRAME); /* ignore side-effect */
port->icount.brk++;
+ atmel_port->break_active = 1;
+ UART_PUT_IER(port, ATMEL_US_RXBRK);
if (uart_handle_break(port))
goto ignore_char;
+ } else {
+ /*
+ * This is either the end-of-break
+ * condition or we've received at
+ * least one character without RXBRK
+ * being set. In both cases, the next
+ * RXBRK will indicate start-of-break.
+ */
+ UART_PUT_IDR(port, ATMEL_US_RXBRK);
+ status &= ~ATMEL_US_RXBRK;
+ atmel_port->break_active = 0;
}
if (status & ATMEL_US_PARE)
port->icount.parity++;
@@ -352,6 +370,16 @@ static irqreturn_t atmel_interrupt(int irq, void *dev_id)
/* Interrupt receive */
if (pending & ATMEL_US_RXRDY)
atmel_rx_chars(port);
+ else if (pending & ATMEL_US_RXBRK) {
+ /*
+ * End of break detected. If it came along
+ * with a character, atmel_rx_chars will
+ * handle it.
+ */
+ UART_PUT_CR(port, ATMEL_US_RSTSTA);
+ UART_PUT_IDR(port, ATMEL_US_RXBRK);
+ atmel_port->break_active = 0;
+ }

// TODO: All reads to CSR will clear these interrupts!
if (pending & ATMEL_US_RIIC) port->icount.rng++;
--
1.4.4.4


2007-06-05 11:24:18

by Haavard Skinnemoen

[permalink] [raw]
Subject: Re: [PATCH] atmel_serial: Fix break handling

On Tue, 5 Jun 2007 14:07:20 +0300
Ivan Kuten <[email protected]> wrote:

> I tried to test your patch on AT91RM9200 with Magic SysRq sequence, unfortunately without
> success - SysRq still does not work. You mention "break count increments" where do you check it ? I have
> cat /proc/tty/driver/atmel_serial
> serinfo:1.0 driver revision:
> 0: uart:ATMEL_SERIAL mmio:0xFEFFF200 irq:1 tx:8554 rx:623 fe:25 RTS|CTS|DTR|DSR|CD|RI
> 1: uart:ATMEL_SERIAL mmio:0xFFFC0000 irq:6 tx:0 rx:0 CTS|DSR|CD|RI
> 2: uart:ATMEL_SERIAL mmio:0xFFFC4000 irq:7 tx:0 rx:0 RI
> 3: uart:ATMEL_SERIAL mmio:0xFFFC8000 irq:8 tx:0 rx:0 DSR|CD|RI
> 4: uart:ATMEL_SERIAL mmio:0xFFFCC000 irq:9 tx:0 rx:0 CTS|DSR|CD|RI
>
> no any break counter.

Andrew Victor pointed out that the RM9200 DBGU doesn't support break at
all, and the data sheet seems to agree. The break counter seems to show
up after the first break has been received and you're probably not
receiving any.

Is it possible for you to try a different USART as console?

Haavard

2007-06-05 11:32:37

by Ivan Kuten

[permalink] [raw]
Subject: Re: [PATCH] atmel_serial: Fix break handling

On Thu, 31 May 2007 21:31:31 +0200
Haavard Skinnemoen wrote:

> It's been almost a year since the last patch I sent attempting to fix
> this. Sorry for not following up any sooner.
>
> Anyway, here's a new attempt. It should work even if we miss some
> interrupts, and it should not break break handling by ignoring the
> return value from uart_handle_break() as Russell pointed out.
>
> There may be cases which aren't handled well: If you send two breaks
> with no characters in between and we miss an interrupt on the first
> one, the driver probably gets a bit confused. I don't see any way to
> fix this, but the driver should get un-confused upon reception of the
> next character.
>
> I've tested this with Magic SysRq on ATSTK1000: It works with this
> patch but not without. Ivan Kuten reported in a different thread that
> SysRq didn't work on AT91RM9200 and I very much doubt it works on
> AT91SAM926x.
>
> The break count in /proc/tty/driver/atmel_serial also makes more sense
> with this patch applied; without it, the break count increments by two
> every time I send a break.
>
> drivers/serial/atmel_serial.c | 32 ++++++++++++++++++++++++++++++--
> 1 files changed, 30 insertions(+), 2 deletions(-)
>

Hi Haavard,

I tried to test your patch on AT91RM9200 with Magic SysRq sequence, unfortunately without
success - SysRq still does not work. You mention "break count increments" where do you check it ? I have
cat /proc/tty/driver/atmel_serial
serinfo:1.0 driver revision:
0: uart:ATMEL_SERIAL mmio:0xFEFFF200 irq:1 tx:8554 rx:623 fe:25 RTS|CTS|DTR|DSR|CD|RI
1: uart:ATMEL_SERIAL mmio:0xFFFC0000 irq:6 tx:0 rx:0 CTS|DSR|CD|RI
2: uart:ATMEL_SERIAL mmio:0xFFFC4000 irq:7 tx:0 rx:0 RI
3: uart:ATMEL_SERIAL mmio:0xFFFC8000 irq:8 tx:0 rx:0 DSR|CD|RI
4: uart:ATMEL_SERIAL mmio:0xFFFCC000 irq:9 tx:0 rx:0 CTS|DSR|CD|RI

no any break counter.

BR,
Ivan



2007-06-18 10:18:48

by Ivan Kuten

[permalink] [raw]
Subject: Re: [PATCH] atmel_serial: Fix break handling

On Tue, 5 Jun 2007 13:23:36 +0200
Haavard Skinnemoen wrote:

> On Tue, 5 Jun 2007 14:07:20 +0300
> Ivan Kuten <[email protected]> wrote:
>
> > I tried to test your patch on AT91RM9200 with Magic SysRq sequence, unfortunately without
> > success - SysRq still does not work. You mention "break count increments" where do you check it ? I have
> > cat /proc/tty/driver/atmel_serial
> > serinfo:1.0 driver revision:
> > 0: uart:ATMEL_SERIAL mmio:0xFEFFF200 irq:1 tx:8554 rx:623 fe:25 RTS|CTS|DTR|DSR|CD|RI
> > 1: uart:ATMEL_SERIAL mmio:0xFFFC0000 irq:6 tx:0 rx:0 CTS|DSR|CD|RI
> > 2: uart:ATMEL_SERIAL mmio:0xFFFC4000 irq:7 tx:0 rx:0 RI
> > 3: uart:ATMEL_SERIAL mmio:0xFFFC8000 irq:8 tx:0 rx:0 DSR|CD|RI
> > 4: uart:ATMEL_SERIAL mmio:0xFFFCC000 irq:9 tx:0 rx:0 CTS|DSR|CD|RI
> >
> > no any break counter.
>
> Andrew Victor pointed out that the RM9200 DBGU doesn't support break at
> all, and the data sheet seems to agree. The break counter seems to show
> up after the first break has been received and you're probably not
> receiving any.
>
> Is it possible for you to try a different USART as console?
>
> Haavard

Hi Haavard,

I tried /dev/ttyAT3, break appeared but not the way I expected, after: stty -F /dev/ttyAT3 brkint
I get:

cat /proc/tty/driver/atmel_serial
serinfo:1.0 driver revision:
0: uart:ATMEL_SERIAL mmio:0xFEFFF200 irq:1 tx:0 rx:0 CTS|DSR|CD|RI
1: uart:ATMEL_SERIAL mmio:0xFFFC0000 irq:6 tx:0 rx:0 CTS|DSR|CD|RI
2: uart:ATMEL_SERIAL mmio:0xFFFC4000 irq:7 tx:0 rx:0 RI
3: uart:ATMEL_SERIAL mmio:0xFFFC8000 irq:8 tx:1530 rx:115 brk:1 RTS|DTR|DSR|CD|RI
4: uart:ATMEL_SERIAL mmio:0xFFFCC000 irq:9 tx:0 rx:0 DSR|CD|RI

brk is 1 not depending on how many breaks was sent. May be I missunderstood "break" meaning?
I use from minicom: Main Functions : send break.........F .

I inserted printk:

/*
* Control the transmission of a break signal
*/
static void atmel_break_ctl(struct uart_port *port, int break_state)
{
printk(KERN_EMERG "atmel_break_ctl break_state %d", break_state);
if (break_state != 0)
UART_PUT_CR(port, ATMEL_US_STTBRK); /* start break */
else
UART_PUT_CR(port, ATMEL_US_STPBRK); /* stop break */
}

it's also not shown.

BR,
Ivan

2007-06-18 10:34:48

by Russell King

[permalink] [raw]
Subject: Re: [PATCH] atmel_serial: Fix break handling

On Mon, Jun 18, 2007 at 01:21:21PM +0300, Ivan Kuten wrote:
> Hi Haavard,
>
> I tried /dev/ttyAT3, break appeared but not the way I expected, after:
> stty -F /dev/ttyAT3 brkint I get:
>
> cat /proc/tty/driver/atmel_serial
> serinfo:1.0 driver revision:
> 0: uart:ATMEL_SERIAL mmio:0xFEFFF200 irq:1 tx:0 rx:0 CTS|DSR|CD|RI
> 1: uart:ATMEL_SERIAL mmio:0xFFFC0000 irq:6 tx:0 rx:0 CTS|DSR|CD|RI
> 2: uart:ATMEL_SERIAL mmio:0xFFFC4000 irq:7 tx:0 rx:0 RI
> 3: uart:ATMEL_SERIAL mmio:0xFFFC8000 irq:8 tx:1530 rx:115 brk:1 RTS|DTR|DSR|CD|RI
> 4: uart:ATMEL_SERIAL mmio:0xFFFCC000 irq:9 tx:0 rx:0 DSR|CD|RI
>
> brk is 1 not depending on how many breaks was sent. May be I missunderstood
> "break" meaning?
> I use from minicom: Main Functions : send break.........F .

Note that if you want to use magic sysrq on ttyAT3, you need the kernel
console on ttyAT3. Since you successfully received one break event on
ttyAT3, the next character should have caused a sysrq event.

> I inserted printk:
>
> /*
> * Control the transmission of a break signal
^^^^^^^^^^^^
So if you're looking at the reception paths, this clearly isn't it...

> */
> static void atmel_break_ctl(struct uart_port *port, int break_state)
> {
> printk(KERN_EMERG "atmel_break_ctl break_state %d", break_state);
> if (break_state != 0)
> UART_PUT_CR(port, ATMEL_US_STTBRK); /* start break */
> else
> UART_PUT_CR(port, ATMEL_US_STPBRK); /* stop break */
> }
>
> it's also not shown.

... and that explains why.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:

2007-06-19 10:33:26

by Ivan Kuten

[permalink] [raw]
Subject: Re: [PATCH] atmel_serial: Fix break handling

On Mon, 18 Jun 2007 11:33:54 +0100
Russell King wrote:

> On Mon, Jun 18, 2007 at 01:21:21PM +0300, Ivan Kuten wrote:
> > Hi Haavard,
> >
> > I tried /dev/ttyAT3, break appeared but not the way I expected, after:
> > stty -F /dev/ttyAT3 brkint I get:
> >
> > cat /proc/tty/driver/atmel_serial
> > serinfo:1.0 driver revision:
> > 0: uart:ATMEL_SERIAL mmio:0xFEFFF200 irq:1 tx:0 rx:0 CTS|DSR|CD|RI
> > 1: uart:ATMEL_SERIAL mmio:0xFFFC0000 irq:6 tx:0 rx:0 CTS|DSR|CD|RI
> > 2: uart:ATMEL_SERIAL mmio:0xFFFC4000 irq:7 tx:0 rx:0 RI
> > 3: uart:ATMEL_SERIAL mmio:0xFFFC8000 irq:8 tx:1530 rx:115 brk:1 RTS|DTR|DSR|CD|RI
> > 4: uart:ATMEL_SERIAL mmio:0xFFFCC000 irq:9 tx:0 rx:0 DSR|CD|RI
> >
> > brk is 1 not depending on how many breaks was sent. May be I missunderstood
> > "break" meaning?
> > I use from minicom: Main Functions : send break.........F .
>
> Note that if you want to use magic sysrq on ttyAT3, you need the kernel
> console on ttyAT3. Since you successfully received one break event on
> ttyAT3, the next character should have caused a sysrq event.
>

Yes, I added to kernel command line "console=ttyAT3,115200" .

I do not receive brk event via minicom, break counter sets to 1 only if I issue
this command: stty -F /dev/ttyAT3 brkint

Anyway I try to debug further to locate the problem.

BR,
Ivan

2007-06-19 17:06:13

by Russell King

[permalink] [raw]
Subject: Re: [PATCH] atmel_serial: Fix break handling

On Tue, Jun 19, 2007 at 01:36:05PM +0300, Ivan Kuten wrote:
> On Mon, 18 Jun 2007 11:33:54 +0100
> Russell King wrote:
>
> > On Mon, Jun 18, 2007 at 01:21:21PM +0300, Ivan Kuten wrote:
> > > Hi Haavard,
> > >
> > > I tried /dev/ttyAT3, break appeared but not the way I expected, after:
> > > stty -F /dev/ttyAT3 brkint I get:
> > >
> > > cat /proc/tty/driver/atmel_serial
> > > serinfo:1.0 driver revision:
> > > 0: uart:ATMEL_SERIAL mmio:0xFEFFF200 irq:1 tx:0 rx:0 CTS|DSR|CD|RI
> > > 1: uart:ATMEL_SERIAL mmio:0xFFFC0000 irq:6 tx:0 rx:0 CTS|DSR|CD|RI
> > > 2: uart:ATMEL_SERIAL mmio:0xFFFC4000 irq:7 tx:0 rx:0 RI
> > > 3: uart:ATMEL_SERIAL mmio:0xFFFC8000 irq:8 tx:1530 rx:115 brk:1 RTS|DTR|DSR|CD|RI
> > > 4: uart:ATMEL_SERIAL mmio:0xFFFCC000 irq:9 tx:0 rx:0 DSR|CD|RI
> > >
> > > brk is 1 not depending on how many breaks was sent. May be I missunderstood
> > > "break" meaning?
> > > I use from minicom: Main Functions : send break.........F .
> >
> > Note that if you want to use magic sysrq on ttyAT3, you need the kernel
> > console on ttyAT3. Since you successfully received one break event on
> > ttyAT3, the next character should have caused a sysrq event.
> >
>
> Yes, I added to kernel command line "console=ttyAT3,115200" .
>
> I do not receive brk event via minicom, break counter sets to 1 only if
> I issue this command: stty -F /dev/ttyAT3 brkint

You won't *receive* a break event via minicom - minicom will probably
ignore the "interrupt" signal generated by the break interrupt. But
aren't you _transmitting_ breaks using minicom? Are you running
minicom on both ends?

Anyway, the fact that the break counter incremented to '1' _and_ you've
set ttyAT3 to be a console means that the next character received should
be interpreted as a magic sysrq character.

Also, I note that the code in atmel_serial.c couldn't care less about
the setting of BRKINT as far as incrementing the break counter (and
that's how it should be) so I'm not really sure what's going on with
your setup. BRKINT just sets ATMEL_US_RXBRK in read_status_mask, which
is only applied to the value read from the status register _after_ we
check the status register for a break and increment the break counter.
So I think that the incrementing of the break counter after you set
BRKINT is just co-incidence.

In any case, I think you need to take more care when composing your
emails. I'm getting really confused as to what you're running at
each end of your serial link and what the configuration actually is.

Please take the time to re-describe from the start what your problem
really is, taking care to clearly identify which end is doing what,
what hardware is at each end, how each end is configured, what it's
running and so forth.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: