[PATCH 0/3] at91_serial: Introduction

[PATCH 0/3] at91_serial: Introduction

[Haavard Skinnemoen]

The following 3 patches make the at91_serial driver usable on AVR32.
The last two patches are not really AVR32-specific and may be
considered as general bug fixes.

There are a few bigger changes I want to do to the at91_serial driver.
If you have objections to any of this, please speak up.

The avr32-arch patch in -mm contains copies of a few files in
include/asm-arm/arch-at91, among others at91rm9200_usart.h. This
duplication is really unnecessary, and I suggest we move the file into
drivers/serial so that it can be used by all architectures.

Since at91_serial can be used by devices other than at91, it's really
a bit misnamed. I'd like to rename it to atmel_serial. Would you
accept a huge patch to do that?

There's also a different driver around for the same piece of hardware
that I wrote almost from scratch a couple of years ago, and that's
distributed with the AT32STK1000 BSP. I'm planning to phase out that
driver, but before I do I want to go through it and try to integrate
all the good stuff into the at91_driver.

Another thing: Andrew, are you the official maintainer of this driver?
If not, who is?

Haavard

[PATCH 1/3] at91_serial: support AVR32

[Haavard Skinnemoen]

This makes it possible to select and build the at91_serial driver
on AVR32. It also #ifdefs out some AT91-specific code for AVR32.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 drivers/serial/Kconfig       |   12 ++++++------
 drivers/serial/at91_serial.c |   14 +++++++++++++-
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 5b48ac2..61b5b52 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -301,21 +301,21 @@ config SERIAL_AMBA_PL011_CONSOLE
 
 config SERIAL_AT91
 	bool "AT91RM9200 / AT91SAM9261 serial port support"
-	depends on ARM && (ARCH_AT91RM9200 || ARCH_AT91SAM9261)
+	depends on ARM && (ARCH_AT91RM9200 || ARCH_AT91SAM9261) || AVR32
 	select SERIAL_CORE
 	help
 	  This enables the driver for the on-chip UARTs of the Atmel
-	  AT91RM9200 and AT91SAM926 processor.
+	  AT91RM9200, AT91SAM926 and AT32AP7000 processors.
 
 config SERIAL_AT91_CONSOLE
 	bool "Support for console on AT91RM9200 / AT91SAM9261 serial port"
 	depends on SERIAL_AT91=y
 	select SERIAL_CORE_CONSOLE
 	help
-	  Say Y here if you wish to use a UART on the Atmel AT91RM9200 or
-	  AT91SAM9261 as the system console (the system console is the device
-	  which receives all kernel messages and warnings and which allows
-	  logins in single user mode).
+	  Say Y here if you wish to use a UART on the Atmel AT91RM9200,
+	  AT91SAM9261 or AT32AP7000 as the system console (the system console
+	  is the device which receives all kernel messages and warnings and
+	  which allows logins in single user mode).
 
 config SERIAL_AT91_TTYAT
 	bool "Install as device ttyAT0-4 instead of ttyS0-4"
diff --git a/drivers/serial/at91_serial.c b/drivers/serial/at91_serial.c
index 54c6b2a..f2fecc6 100644
--- a/drivers/serial/at91_serial.c
+++ b/drivers/serial/at91_serial.c
@@ -40,8 +40,10 @@ #include <asm/arch/at91rm9200_usart.h>
 #include <asm/arch/at91rm9200_pdc.h>
 #include <asm/mach/serial_at91.h>
 #include <asm/arch/board.h>
+#ifdef CONFIG_ARM
 #include <asm/arch/system.h>
 #include <asm/arch/gpio.h>
+#endif
 
 #if defined(CONFIG_SERIAL_AT91_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
 #define SUPPORT_SYSRQ
@@ -134,6 +136,7 @@ static void at91_set_mctrl(struct uart_p
 	unsigned int control = 0;
 	unsigned int mode;
 
+#ifdef CONFIG_ARM
 	if (arch_identify() == ARCH_ID_AT91RM9200) {
 		/*
 		 * AT91RM9200 Errata #39: RTS0 is not internally connected to PA21.
@@ -146,6 +149,7 @@ static void at91_set_mctrl(struct uart_p
 				at91_set_gpio_value(AT91_PIN_PA21, 1);
 		}
 	}
+#endif
 
 	if (mctrl & TIOCM_RTS)
 		control |= AT91_US_RTSEN;
@@ -611,7 +615,8 @@ static int at91_request_port(struct uart
 		return -EBUSY;
 
 	if (port->flags & UPF_IOREMAP) {
-		port->membase = ioremap(port->mapbase, size);
+		if (port->membase == NULL)
+			port->membase = ioremap(port->mapbase, size);
 		if (port->membase == NULL) {
 			release_mem_region(port->mapbase, size);
 			return -ENOMEM;
@@ -693,12 +698,19 @@ static void __devinit at91_init_port(str
 	port->mapbase	= pdev->resource[0].start;
 	port->irq	= pdev->resource[1].start;
 
+#ifdef CONFIG_AVR32
+	port->flags |= UPF_IOREMAP;
+	port->membase = ioremap(pdev->resource[0].start,
+				pdev->resource[0].end
+				- pdev->resource[0].start + 1);
+#else
 	if (port->mapbase == AT91_VA_BASE_SYS + AT91_DBGU)		/* Part of system perpherals - already mapped */
 		port->membase = (void __iomem *) port->mapbase;
 	else {
 		port->flags	|= UPF_IOREMAP;
 		port->membase	= NULL;
 	}
+#endif
 
 	if (!at91_port->clk) {		/* for console, the clock could already be configured */
 		at91_port->clk = clk_get(&pdev->dev, "usart");
-- 
1.4.0

[PATCH 2/3] at91_serial: Fix break handling

[Haavard Skinnemoen]

The RXBRK field in the AT91/AT32 USART status register has the
following definition according to 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.

With this patch, SysRq works as expected on the AT32STK1000 board
with an AT32AP7000 CPU.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 drivers/serial/at91_serial.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/at91_serial.c b/drivers/serial/at91_serial.c
index f2fecc6..c759e7c 100644
--- a/drivers/serial/at91_serial.c
+++ b/drivers/serial/at91_serial.c
@@ -112,6 +112,7 @@ struct at91_uart_port {
 	struct uart_port	uart;		/* uart */
 	struct clk		*clk;		/* uart clock */
 	unsigned short		suspended;	/* is port suspended? */
+	unsigned short		break_active;
 };
 
 static struct at91_uart_port at91_ports[AT91_NR_UART];
@@ -250,6 +251,7 @@ static void at91_break_ctl(struct uart_p
  */
 static void at91_rx_chars(struct uart_port *port, struct pt_regs *regs)
 {
+	struct at91_uart_port *at91_port = (struct at91_uart_port *) port;
 	struct tty_struct *tty = port->info->tty;
 	unsigned int status, ch, flg;
 
@@ -269,9 +271,14 @@ static void at91_rx_chars(struct uart_po
 			UART_PUT_CR(port, AT91_US_RSTSTA);	/* clear error */
 			if (status & AT91_US_RXBRK) {
 				status &= ~(AT91_US_PARE | AT91_US_FRAME);	/* ignore side-effect */
-				port->icount.brk++;
-				if (uart_handle_break(port))
+				if (at91_port->break_active) {
+					at91_port->break_active = 0;
+				} else {
+					at91_port->break_active = 1;
+					port->icount.brk++;
+					uart_handle_break(port);
 					goto ignore_char;
+				}
 			}
 			if (status & AT91_US_PARE)
 				port->icount.parity++;
-- 
1.4.0

[PATCH 3/3] at91_serial: Fix roundoff error in at91_console_get_options

[Haavard Skinnemoen]

The at91_console_get_options() function initializes the baud,
parity and bits settings from the actual hardware setup, in
case it has been initialized by a e.g. boot loader.

The baud rate, however, is not necessarily exactly equal to one of
the standard baud rates (115200, etc.) This means that the baud rate
calculated by this function may be slightly higher or slightly lower
than one of the standard baud rates.

If the baud rate is slightly lower than the target, this causes
problems when uart_set_option() tries to match the detected baud rate
against the standard baud rate, as it will always select a baud rate
that is lower or equal to the target rate. For example if the
detected baud rate is slightly lower than 115200, usart_set_options()
will select 57600.

This patch fixes the problem by subtracting 1 from the value in BRGR
when calculating the baud rate. The detected baud rate will thus
always be higher than the nearest standard baud rate, and
uart_set_options() will end up doing the right thing.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 drivers/serial/at91_serial.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/at91_serial.c b/drivers/serial/at91_serial.c
index c759e7c..528d717 100644
--- a/drivers/serial/at91_serial.c
+++ b/drivers/serial/at91_serial.c
@@ -806,8 +806,14 @@ static void __init at91_console_get_opti
 	else if (mr == AT91_US_PAR_ODD)
 		*parity = 'o';
 
+	/*
+	 * The serial core only rounds down when matching this to a
+	 * supported baud rate. Make sure we don't end up slightly
+	 * lower than one of those, as it would make us fall through
+	 * to a much lower baud rate than we really want.
+	 */
 	quot = UART_GET_BRGR(port);
-	*baud = port->uartclk / (16 * (quot));
+	*baud = port->uartclk / (16 * (quot - 1));
 }
 
 static int __init at91_console_setup(struct console *co, char *options)
-- 
1.4.0

Re: [PATCH 1/3] at91_serial: support AVR32

[Russell King]

On Wed, Aug 02, 2006 at 04:51:46PM +0200, Haavard Skinnemoen wrote:
> diff --git a/drivers/serial/at91_serial.c b/drivers/serial/at91_serial.c
> index 54c6b2a..f2fecc6 100644
> --- a/drivers/serial/at91_serial.c
> +++ b/drivers/serial/at91_serial.c
> @@ -40,8 +40,10 @@ #include <asm/arch/at91rm9200_usart.h>
>  #include <asm/arch/at91rm9200_pdc.h>
>  #include <asm/mach/serial_at91.h>
>  #include <asm/arch/board.h>
> +#ifdef CONFIG_ARM
>  #include <asm/arch/system.h>

I'd rather this file wasn't included in any drivers in any case.

> @@ -611,7 +615,8 @@ static int at91_request_port(struct uart
>  		return -EBUSY;
>  
>  	if (port->flags & UPF_IOREMAP) {
> -		port->membase = ioremap(port->mapbase, size);
> +		if (port->membase == NULL)
> +			port->membase = ioremap(port->mapbase, size);

This change makes no sense.  If you don't want ioremap, don't set UPF_IOREMAP.

>  		if (port->membase == NULL) {
>  			release_mem_region(port->mapbase, size);
>  			return -ENOMEM;
> @@ -693,12 +698,19 @@ static void __devinit at91_init_port(str
>  	port->mapbase	= pdev->resource[0].start;
>  	port->irq	= pdev->resource[1].start;
>  
> +#ifdef CONFIG_AVR32
> +	port->flags |= UPF_IOREMAP;
> +	port->membase = ioremap(pdev->resource[0].start,
> +				pdev->resource[0].end
> +				- pdev->resource[0].start + 1);

Don't see the requirement for this.

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

Re: [PATCH 2/3] at91_serial: Fix break handling

[Russell King]

On Wed, Aug 02, 2006 at 04:51:47PM +0200, Haavard Skinnemoen wrote:
> @@ -269,9 +271,14 @@ static void at91_rx_chars(struct uart_po
>  			UART_PUT_CR(port, AT91_US_RSTSTA);	/* clear error */
>  			if (status & AT91_US_RXBRK) {
>  				status &= ~(AT91_US_PARE | AT91_US_FRAME);	/* ignore side-effect */
> -				port->icount.brk++;
> -				if (uart_handle_break(port))
> +				if (at91_port->break_active) {
> +					at91_port->break_active = 0;
> +				} else {
> +					at91_port->break_active = 1;
> +					port->icount.brk++;
> +					uart_handle_break(port);
>  					goto ignore_char;
> +				}

Two points here.

1. Effectively, this just ignores every second break status.  We've no idea
   _which_ break interrupt is going to be ignored.
2. it breaks break handling.  uart_handle_break returns a value for a
   reason.  Use it - don't unconditionally ignore the received character.

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

Re: [PATCH 1/3] at91_serial: support AVR32

[Haavard Skinnemoen]

On Wed, 2 Aug 2006 16:15:05 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Wed, Aug 02, 2006 at 04:51:46PM +0200, Haavard Skinnemoen wrote:
> > diff --git a/drivers/serial/at91_serial.c
> > b/drivers/serial/at91_serial.c index 54c6b2a..f2fecc6 100644
> > --- a/drivers/serial/at91_serial.c
> > +++ b/drivers/serial/at91_serial.c
> > @@ -40,8 +40,10 @@ #include <asm/arch/at91rm9200_usart.h>
> >  #include <asm/arch/at91rm9200_pdc.h>
> >  #include <asm/mach/serial_at91.h>
> >  #include <asm/arch/board.h>
> > +#ifdef CONFIG_ARM
> >  #include <asm/arch/system.h>
> 
> I'd rather this file wasn't included in any drivers in any case.

I suppose the cpu_is_*() stuff I've heard about through David Brownell
is going to solve that. Could someone point me at the patch
implementing it for ARM so that I can implement something similar for
AVR32?

> > @@ -611,7 +615,8 @@ static int at91_request_port(struct uart
> >  		return -EBUSY;
> >  
> >  	if (port->flags & UPF_IOREMAP) {
> > -		port->membase = ioremap(port->mapbase, size);
> > +		if (port->membase == NULL)
> > +			port->membase = ioremap(port->mapbase,
> > size);
> 
> This change makes no sense.  If you don't want ioremap, don't set
> UPF_IOREMAP.

It's supposed to prevent it from ioremap()ing it again if it was
previously ioremap()ed for use by the console. I'll drop it along with
the call to ioremap() in the next hunk.

> >  		if (port->membase == NULL) {
> >  			release_mem_region(port->mapbase, size);
> >  			return -ENOMEM;
> > @@ -693,12 +698,19 @@ static void __devinit at91_init_port(str
> >  	port->mapbase	= pdev->resource[0].start;
> >  	port->irq	= pdev->resource[1].start;
> >  
> > +#ifdef CONFIG_AVR32
> > +	port->flags |= UPF_IOREMAP;
> > +	port->membase = ioremap(pdev->resource[0].start,
> > +				pdev->resource[0].end
> > +				- pdev->resource[0].start + 1);
> 
> Don't see the requirement for this.

Well, maybe I misunderstood something. The reason I added it was to get
a pointer to the USART registers for use by the serial console. I guess
it would work equally well to just set UPF_IOREMAP, except that the
console will start working a bit later...

What is the best way to get the serial console up and running early in
the boot process? ioremap() only happens to work because the internal
peripherals are permanently mapped on AVR32, so I shouldn't really
depend on that.

Haavard

Re: [PATCH 1/3] at91_serial: support AVR32

[Russell King]

On Wed, Aug 02, 2006 at 06:00:23PM +0200, Haavard Skinnemoen wrote:
> What is the best way to get the serial console up and running early in
> the boot process?

There is no generic solution to this problem other than to put up with
the late initialisation of serial console.

If you want a serial console earlier, have a look at how the 8250_early
stuff works.

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

Re: [PATCH 2/3] at91_serial: Fix break handling

[Haavard Skinnemoen]

On Wed, 2 Aug 2006 16:17:41 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:


> 1. Effectively, this just ignores every second break status.  We've
> no idea _which_ break interrupt is going to be ignored.

Good point. Would it be better if I forced break_active to zero after
some timeout?

Come to think about it, it's really strange that there's a single bit
indicating both start-of-break and end-of-break. I'll see if I can find
a way to tell the difference.

> 2. it breaks break handling.  uart_handle_break returns a value for a
>    reason.  Use it - don't unconditionally ignore the received
> character.

Ok, I'll fix it.

Out of curiosity, why does it return a value? ;)

Håvard

Re: [PATCH 2/3] at91_serial: Fix break handling

[Russell King]

On Wed, Aug 02, 2006 at 06:14:03PM +0200, Haavard Skinnemoen wrote:
> On Wed, 2 Aug 2006 16:17:41 +0100
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > 2. it breaks break handling.  uart_handle_break returns a value for a
> >    reason.  Use it - don't unconditionally ignore the received
> > character.
> 
> Ok, I'll fix it.
> 
> Out of curiosity, why does it return a value? ;)

Because you may or may not need to ignore the received character!

When a break condition occurs which is not ignored by the termios
settings, you need to insert a TTY_BREAK indicator into the tty
received queue, so that the tty layers and userspace can do the
right thing.  There is the requirement for errors to be passed to
userspace if userspace has requested that behaviour.

However, if this is the serial console with sysrq support, then break
is masked by that and needs to be ignored.

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

Re: [PATCH 1/3] at91_serial: support AVR32

[Haavard Skinnemoen]

On Wed, 2 Aug 2006 17:03:53 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Wed, Aug 02, 2006 at 06:00:23PM +0200, Haavard Skinnemoen wrote:
> > What is the best way to get the serial console up and running early
> > in the boot process?
> 
> There is no generic solution to this problem other than to put up with
> the late initialisation of serial console.
> 
> If you want a serial console earlier, have a look at how the
> 8250_early stuff works.

Ok, thanks. I'll have a look at it later.

Here's an updated patch.

Haavard

From: Haavard Skinnemoen <hskinnemoen@atmel.com>
Date: Wed, 2 Aug 2006 14:59:13 +0200
Subject: [PATCH 1/3] at91_serial: support AVR32

This makes it possible to select and build the at91_serial driver
on AVR32. It also #ifdefs out some AT91-specific code for AVR32.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 drivers/serial/Kconfig       |   12 ++++++------
 drivers/serial/at91_serial.c |    9 +++++++++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index 5b48ac2..61b5b52 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -301,21 +301,21 @@ config SERIAL_AMBA_PL011_CONSOLE
 
 config SERIAL_AT91
 	bool "AT91RM9200 / AT91SAM9261 serial port support"
-	depends on ARM && (ARCH_AT91RM9200 || ARCH_AT91SAM9261)
+	depends on ARM && (ARCH_AT91RM9200 || ARCH_AT91SAM9261) || AVR32
 	select SERIAL_CORE
 	help
 	  This enables the driver for the on-chip UARTs of the Atmel
-	  AT91RM9200 and AT91SAM926 processor.
+	  AT91RM9200, AT91SAM926 and AT32AP7000 processors.
 
 config SERIAL_AT91_CONSOLE
 	bool "Support for console on AT91RM9200 / AT91SAM9261 serial port"
 	depends on SERIAL_AT91=y
 	select SERIAL_CORE_CONSOLE
 	help
-	  Say Y here if you wish to use a UART on the Atmel AT91RM9200 or
-	  AT91SAM9261 as the system console (the system console is the device
-	  which receives all kernel messages and warnings and which allows
-	  logins in single user mode).
+	  Say Y here if you wish to use a UART on the Atmel AT91RM9200,
+	  AT91SAM9261 or AT32AP7000 as the system console (the system console
+	  is the device which receives all kernel messages and warnings and
+	  which allows logins in single user mode).
 
 config SERIAL_AT91_TTYAT
 	bool "Install as device ttyAT0-4 instead of ttyS0-4"
diff --git a/drivers/serial/at91_serial.c b/drivers/serial/at91_serial.c
index 54c6b2a..bee81ff 100644
--- a/drivers/serial/at91_serial.c
+++ b/drivers/serial/at91_serial.c
@@ -40,8 +40,10 @@ #include <asm/arch/at91rm9200_usart.h>
 #include <asm/arch/at91rm9200_pdc.h>
 #include <asm/mach/serial_at91.h>
 #include <asm/arch/board.h>
+#ifdef CONFIG_ARM
 #include <asm/arch/system.h>
 #include <asm/arch/gpio.h>
+#endif
 
 #if defined(CONFIG_SERIAL_AT91_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
 #define SUPPORT_SYSRQ
@@ -134,6 +136,7 @@ static void at91_set_mctrl(struct uart_p
 	unsigned int control = 0;
 	unsigned int mode;
 
+#ifdef CONFIG_ARM
 	if (arch_identify() == ARCH_ID_AT91RM9200) {
 		/*
 		 * AT91RM9200 Errata #39: RTS0 is not internally connected to PA21.
@@ -146,6 +149,7 @@ static void at91_set_mctrl(struct uart_p
 				at91_set_gpio_value(AT91_PIN_PA21, 1);
 		}
 	}
+#endif
 
 	if (mctrl & TIOCM_RTS)
 		control |= AT91_US_RTSEN;
@@ -693,12 +697,17 @@ static void __devinit at91_init_port(str
 	port->mapbase	= pdev->resource[0].start;
 	port->irq	= pdev->resource[1].start;
 
+#ifdef CONFIG_AVR32
+	port->flags |= UPF_IOREMAP;
+	port->membase = NULL;
+#else
 	if (port->mapbase == AT91_VA_BASE_SYS + AT91_DBGU)		/* Part of system perpherals - already mapped */
 		port->membase = (void __iomem *) port->mapbase;
 	else {
 		port->flags	|= UPF_IOREMAP;
 		port->membase	= NULL;
 	}
+#endif
 
 	if (!at91_port->clk) {		/* for console, the clock could already be configured */
 		at91_port->clk = clk_get(&pdev->dev, "usart");
-- 
1.4.0

Re: [PATCH 2/3] at91_serial: Fix break handling

[Haavard Skinnemoen]

On Wed, 2 Aug 2006 16:17:41 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> 1. Effectively, this just ignores every second break status.  We've
> no idea _which_ break interrupt is going to be ignored.
> 2. it breaks break handling.  uart_handle_break returns a value for a
>    reason.  Use it - don't unconditionally ignore the received
> character.

Here's another attempt, which should at least fix #2 and hopefully #1.
If a break appears to have been active for more than one second, we
assume it's a new start-of-break.

Tested using SysRq on AT32AP7000. Seems to work well when dumping lots
of text to the console.

Haavard

From: Haavard Skinnemoen <hskinnemoen@atmel.com>
Date: Tue, 4 Jul 2006 10:25:59 +0200
Subject: [PATCH] at91_serial: Fix break handling

The RXBRK field in the AT91/AT32 USART status register has the
following definition according to 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.

Also, if an end-of-break appears more than one second after
start-of-break, assume that we missed the last one and treat it as a
start-of-break.

With this patch, SysRq works as expected on the AT32STK1000 board
with an AT32AP7000 CPU.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 drivers/serial/at91_serial.c |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/serial/at91_serial.c b/drivers/serial/at91_serial.c
index bee81ff..484faa2 100644
--- a/drivers/serial/at91_serial.c
+++ b/drivers/serial/at91_serial.c
@@ -112,6 +112,8 @@ struct at91_uart_port {
 	struct uart_port	uart;		/* uart */
 	struct clk		*clk;		/* uart clock */
 	unsigned short		suspended;	/* is port suspended? */
+	unsigned short		break_active;	/* currently receiving break */
+	unsigned long		break_timeout;	/* when to stop waiting for end-of-break */
 };
 
 static struct at91_uart_port at91_ports[AT91_NR_UART];
@@ -250,6 +252,7 @@ static void at91_break_ctl(struct uart_p
  */
 static void at91_rx_chars(struct uart_port *port, struct pt_regs *regs)
 {
+	struct at91_uart_port *at91_port = (struct at91_uart_port *) port;
 	struct tty_struct *tty = port->info->tty;
 	unsigned int status, ch, flg;
 
@@ -269,9 +272,18 @@ static void at91_rx_chars(struct uart_po
 			UART_PUT_CR(port, AT91_US_RSTSTA);	/* clear error */
 			if (status & AT91_US_RXBRK) {
 				status &= ~(AT91_US_PARE | AT91_US_FRAME);	/* ignore side-effect */
-				port->icount.brk++;
-				if (uart_handle_break(port))
-					goto ignore_char;
+				if (at91_port->break_active
+				    && time_before(jiffies,
+						   at91_port->break_timeout)) {
+					at91_port->break_active = 0;
+					status &= ~AT91_US_RXBRK;
+				} else {
+					at91_port->break_active = 1;
+					at91_port->break_timeout = jiffies + HZ;
+					port->icount.brk++;
+					if (uart_handle_break(port))
+						goto ignore_char;
+				}
 			}
 			if (status & AT91_US_PARE)
 				port->icount.parity++;
-- 
1.4.0

Re: [PATCH 0/3] at91_serial: Introduction

[Russell King]

On Wed, Aug 02, 2006 at 04:51:45PM +0200, Haavard Skinnemoen wrote:
> Another thing: Andrew, are you the official maintainer of this driver?
> If not, who is?

I've not heard from Andrew, so I'm not sure what to do about this.  I
think these changes need validating by someone with the existing driver's
hardware (iow, AT91RM9200 and/or AT91SAM9261) so we can be sure we don't
break that support.

Andrew?

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

Re: [PATCH 0/3] at91_serial: Introduction

[Haavard Skinnemoen]

On Sat, 23 Sep 2006 22:14:17 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Wed, Aug 02, 2006 at 04:51:45PM +0200, Haavard Skinnemoen wrote:
> > Another thing: Andrew, are you the official maintainer of this
> > driver? If not, who is?
> 
> I've not heard from Andrew, so I'm not sure what to do about this.  I
> think these changes need validating by someone with the existing
> driver's hardware (iow, AT91RM9200 and/or AT91SAM9261) so we can be
> sure we don't break that support.

I really want at least the first one to go in, as the new AVR32 port
would soon find itself without a serial driver otherwise. And it
doesn't make any actual changes for the CONFIG_ARM case, so it should
be quite safe. The last two are bugfixes which I believe make sense on
ARM as well, but that's also a reason why they should be more thoroughly
tested.

I can resend them as individual patches if it helps make it more clear
that they don't really depend on each other.

I have a AT91RM9200-EK board lying around, so I might be able to test
the patches on that as soon as I get the necessary cross compilers and
other tools set up.

Haavard

Re: [PATCH 0/3] at91_serial: Introduction

[Russell King]

On Mon, Sep 25, 2006 at 02:01:23PM +0200, Haavard Skinnemoen wrote:
> I can resend them as individual patches if it helps make it more clear
> that they don't really depend on each other.

If you think that will help.

> I have a AT91RM9200-EK board lying around, so I might be able to test
> the patches on that as soon as I get the necessary cross compilers and
> other tools set up.

If you need toolchains, then they're already available on the net for
easy download, as Lennert recently pointed out:

|  As I regularly test with different gcc versions and encourage others
|  to do the same, I've had a set available for a while at:
|
|        http://www.wantstofly.org/~buytenh/kernel/arm-cross/
|
|  (Generated with crosstool 0.42, see http://kegel.com/crosstool)  Any
|  suggestions for making them easier to find for folks?

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

Re: [PATCH 0/3] at91_serial: Introduction

[Andrew Victor]

hi,

> On Wed, Aug 02, 2006 at 04:51:45PM +0200, Haavard Skinnemoen wrote:
> > Another thing: Andrew, are you the official maintainer of this driver?
> > If not, who is?
> 
> I've not heard from Andrew, so I'm not sure what to do about this.  I
> think these changes need validating by someone with the existing driver's
> hardware (iow, AT91RM9200 and/or AT91SAM9261) so we can be sure we don't
> break that support.

For patch 1, I'm not to keen on the:

  +#ifdef CONFIG_AVR32
  +       port->flags |= UPF_IOREMAP;
  +       port->membase = ioremap(pdev->resource[0].start,
  +                               pdev->resource[0].end
  +                               - pdev->resource[0].start + 1);
  +#else

part.  It might be better to pass a flag (in the platform_data
structure) whether we are providing a virtual or a physical address.
(If you want early init on the serial console, then I recommend just
using a static mapping for the DBGU peripheral).

Patch 2 & 3 look correct, but they would need to be tested.


Regards,
  Andrew Victor

Re: [PATCH 0/3] at91_serial: Introduction

[Haavard Skinnemoen]

On 26 Sep 2006 11:06:24 +0200
Andrew Victor <andrew@sanpeople.com> wrote:

> For patch 1, I'm not to keen on the:
> 
>   +#ifdef CONFIG_AVR32
>   +       port->flags |= UPF_IOREMAP;
>   +       port->membase = ioremap(pdev->resource[0].start,
>   +                               pdev->resource[0].end
>   +                               - pdev->resource[0].start + 1);
>   +#else
> 
> part.  It might be better to pass a flag (in the platform_data
> structure) whether we are providing a virtual or a physical address.
> (If you want early init on the serial console, then I recommend just
> using a static mapping for the DBGU peripheral).

I sent a new patch in the same thread with this instead:

+#ifdef CONFIG_AVR32
+	port->flags |= UPF_IOREMAP;
+	port->membase = NULL;
+#else

This means that the console will be initialized a bit late, but I can
live with that for now. Maybe we can agree on a platform_data format so
that we can remove the #ifdef altogether?

> Patch 2 & 3 look correct, but they would need to be tested.

Yeah, I'm struggling a bit with SPI right now, but I'll se if I can get
my AT91 board up and running after that.

I'll resend the first patch when the AVR32 patches are in, and the
last two after I've tested them on AT91.

Haavard

Re: [PATCH 0/3] at91_serial: Introduction

[Andrew Victor]

hi Haavard,

> Maybe we can agree on a platform_data format so
> that we can remove the #ifdef altogether?

The platform_data structure is currently defined in
include/asm-arm/arch-at91rm9200/board.h as:

struct at91_uart_data {
	short	use_dma_tx;	/* use transmit DMA? */
	short	use_dma_rx;	/* use receive DMA? */
};

I don't think the DMA-support is currently in mainline, but is in the
pending patches on http://maxim.org.za/AT91RM9200/2.6/

I guess we can just add another field:
	short	no_remap;	/* base address is already mapped */
(or something similar)


Regards,
  Andrew Victor

Re: [PATCH 0/3] at91_serial: Introduction

[Haavard Skinnemoen]

On 26 Sep 2006 11:28:11 +0200
Andrew Victor <andrew@sanpeople.com> wrote:

> hi Haavard,
> 
> > Maybe we can agree on a platform_data format so
> > that we can remove the #ifdef altogether?
> 
> The platform_data structure is currently defined in
> include/asm-arm/arch-at91rm9200/board.h as:
> 
> struct at91_uart_data {
> 	short	use_dma_tx;	/* use transmit DMA? */
> 	short	use_dma_rx;	/* use receive DMA? */
> };
> 
> I don't think the DMA-support is currently in mainline, but is in the
> pending patches on http://maxim.org.za/AT91RM9200/2.6/

Are you going to submit it for 2.6.19? I want to try to slam a big
rename patch in without messing up too many not-yet-submitted patches...

> I guess we can just add another field:
> 	short	no_remap;	/* base address is already
> mapped */ (or something similar)

Or maybe even better:
	void __iomem *regs;	/* fixed mapping of base address */

to indicate the actual mapping (if it's NULL, it hasn't been mapped yet)

Haavard

Re: [PATCH 0/3] at91_serial: Introduction

[Andrew Victor]

hi Haavard,

> > I don't think the DMA-support is currently in mainline, but is in the
> > pending patches on http://maxim.org.za/AT91RM9200/2.6/
> 
> Are you going to submit it for 2.6.19? I want to try to slam a big
> rename patch in without messing up too many not-yet-submitted patches...

I was intending to, but Chip Coldwell hasn't gotten back to me yet about
some problems that were reported with the DMA support.

So I guess now would be as good a time as any to rename the serial
driver.


Regards, 
  Andrew Victor

Re: [PATCH 0/3] at91_serial: Introduction

[Haavard Skinnemoen]

On 26 Sep 2006 11:06:24 +0200
Andrew Victor <andrew@sanpeople.com> wrote:

> Patch 2 & 3 look correct, but they would need to be tested.

Ok, I've tested part 2 (at91_serial: Fix roundoff error in
at91_console_get_options), and it works on my AT91RM9200-EK board. In
fact, the latest git code does _not_ work -- I see the same garbage as
on ATSTK1000, and this patch fixes it.

Part 3 probably has real problems, and I'm not sure how to fix it. I'll
try some kind of "break timeout" so that any problems will at least be
transient. The current break code works on neither STK1000 nor
AT91RM9200-EK, so even a not-quite-100%-correct fix should be
acceptable, no?

I'm queuing up some patches now that I'll send off as soon as I get them
tested on both ARM and AVR32. This includes the big rename patch I've
been talking about.

Håvard