Re: Merging Skylark's IOC3 patch

[Russell King <rmk@arm.linux.org.uk>]

On Sun, Feb 19, 2006 at 09:57:40PM +0000, Martin Michlmayr wrote:
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -310,6 +310,9 @@ static unsigned int serial_in(struct uar
>  	case UPIO_MEM:
>  		return readb(up->port.membase + offset);
>  
> +	case UPIO_IOC3:
> +		return readb(up->port.membase + (offset^3));
> +
>  	case UPIO_MEM32:
>  		return readl(up->port.membase + offset);
>  
> @@ -338,6 +341,10 @@ serial_out(struct uart_8250_port *up, in
>  		writeb(value, up->port.membase + offset);
>  		break;
>  
> +	case UPIO_IOC3:
> +		writeb(value, up->port.membase + (offset^3));
> +		break;
> +
>  	case UPIO_MEM32:
>  		writel(value, up->port.membase + offset);
>  		break;

Hmm, this starts to open up the question of whether having an array
of register pointers becomes cheaper.

> @@ -2300,8 +2311,10 @@ int __init serial8250_start_console(stru
>  
>  	add_preferred_console("ttyS", line, options);
>  	printk("Adding console on ttyS%d at %s 0x%lx (options '%s')\n",
> -		line, port->iotype == UPIO_MEM ? "MMIO" : "I/O port",
> -		port->iotype == UPIO_MEM ? (unsigned long) port->mapbase :
> +		line, port->iotype == UPIO_MEM ? "MMIO" : 
> +		port->iotype == UPIO_IOC3 ? "IOC3" : "I/O port",
> +		(port->iotype == UPIO_MEM || port->iotype == UPIO_IOC3) ?
> +		    (unsigned long) port->mapbase :
>  		    (unsigned long) port->iobase, options);

Maybe the "iotype" should index an array?

> diff --git a/include/linux/serial.h b/include/linux/serial.h
> index c69c6b9..a3ee515 100644
> --- a/include/linux/serial.h
> +++ b/include/linux/serial.h
> @@ -82,6 +82,7 @@ struct serial_struct {
>  #define SERIAL_IO_PORT	0
>  #define SERIAL_IO_HUB6	1
>  #define SERIAL_IO_MEM	2
> +#define SERIAL_IO_IOC3	3
>  
>  struct serial_uart_config {
>  	char	*name;
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 4041122..9441c90 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -221,6 +221,7 @@ struct uart_port {
>  #define UPIO_MEM		(2)
>  #define UPIO_MEM32		(3)
>  #define UPIO_AU			(4)			/* Au1x00 type IO */
> +#define UPIO_IOC3		(5)

Comparing SERIAL_IO_* and UPIO_*, I have to say no here.  Always ensure
that they end up with the same number.  SERIAL_IO_* is the current
userland version of UPIO_*.  At some point in the future (once the
pipedream of having _all_ serial drivers updated becomes reality) this
can be sorted out sanely.

> +	port.membase = (unsigned char *) &idd->vma->sregs.uarta;
> +	port.mapbase = ((unsigned long) port.membase) & 0xFFFFFFFFFF;
> +	d->line_a = serial8250_register_port(&port);

mapbase is supposed to be the physical address, whereas membase is what is
used to access the register, and should be something compatible with MMIO
accessors.  Is the cast really required here?

Are you tagging MMIO stuff with __iomem ?

Apart from that, I don't have any further comments at present.

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