Preliminary patch for ip32 ttyS* device

Preliminary patch for ip32 ttyS* device

[Giuseppe Sacco]

Hi all,
this is a patch that make ttyS0 and ttyS1 work on my SGI O2. I don't know if it is enough good for a general use since I am also changing code drivers/serial/serial_core.c. Probably the best solution would be to use mapbase instead of membase in arch/mips/sgi-ip32/ip32-platform.c.

Any comments?

Bye,
Giuseppe

---
diff --git a/arch/mips/sgi-ip32/ip32-platform.c b/arch/mips/sgi-ip32/ip32-platform.c
index 7309e48..77febd6 100644
--- a/arch/mips/sgi-ip32/ip32-platform.c
+++ b/arch/mips/sgi-ip32/ip32-platform.c
@@ -42,7 +42,7 @@ static struct platform_device uart8250_device = {
 static int __init uart8250_init(void)
 {
 	uart8250_data[0].membase = (void __iomem *) &mace->isa.serial1;
-	uart8250_data[1].membase = (void __iomem *) &mace->isa.serial1;
+	uart8250_data[1].membase = (void __iomem *) &mace->isa.serial2;
 
 	return platform_device_register(&uart8250_device);
 }
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 3bb5d24..7caa877 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2455,6 +2455,8 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
 	case UPIO_AU:
 	case UPIO_TSI:
 	case UPIO_DWAPB:
+		if (port1->mapbase==0 && port2->mapbase==0)
+			return (port1->membase == port2->membase);
 		return (port1->mapbase == port2->mapbase);
 	}
 	return 0;

Re: Preliminary patch for ip32 ttyS* device

[Ralf Baechle]

On Tue, Oct 30, 2007 at 09:40:15PM +0100, Giuseppe Sacco wrote:

> this is a patch that make ttyS0 and ttyS1 work on my SGI O2. I don't know if it is enough good for a general use since I am also changing code drivers/serial/serial_core.c. Probably the best solution would be to use mapbase instead of membase in arch/mips/sgi-ip32/ip32-platform.c.

> diff --git a/arch/mips/sgi-ip32/ip32-platform.c b/arch/mips/sgi-ip32/ip32-platform.c
> index 7309e48..77febd6 100644
> --- a/arch/mips/sgi-ip32/ip32-platform.c
> +++ b/arch/mips/sgi-ip32/ip32-platform.c
> @@ -42,7 +42,7 @@ static struct platform_device uart8250_device = {
>  static int __init uart8250_init(void)
>  {
>  	uart8250_data[0].membase = (void __iomem *) &mace->isa.serial1;
> -	uart8250_data[1].membase = (void __iomem *) &mace->isa.serial1;
> +	uart8250_data[1].membase = (void __iomem *) &mace->isa.serial2;

The s/isa.serial1/isa.serial2/ part looks reasonable.

> diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
> index 3bb5d24..7caa877 100644
> --- a/drivers/serial/serial_core.c
> +++ b/drivers/serial/serial_core.c
> @@ -2455,6 +2455,8 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
>  	case UPIO_AU:
>  	case UPIO_TSI:
>  	case UPIO_DWAPB:
> +		if (port1->mapbase==0 && port2->mapbase==0)
> +			return (port1->membase == port2->membase);

This hack is only needed because ->mapbase is not initialized.

  Ralf

Re: Preliminary patch for ip32 ttyS* device

[Giuseppe Sacco]

Il giorno mer, 31/10/2007 alle 13.08 +0000, Ralf Baechle ha scritto:
> On Tue, Oct 30, 2007 at 09:40:15PM +0100, Giuseppe Sacco wrote:
[...]
> > diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
> > index 3bb5d24..7caa877 100644
> > --- a/drivers/serial/serial_core.c
> > +++ b/drivers/serial/serial_core.c
> > @@ -2455,6 +2455,8 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
> >  	case UPIO_AU:
> >  	case UPIO_TSI:
> >  	case UPIO_DWAPB:
> > +		if (port1->mapbase==0 && port2->mapbase==0)
> > +			return (port1->membase == port2->membase);
> 
> This hack is only needed because ->mapbase is not initialized.

Right, for a few days I tried to correctly initialise mapbase and
setting UPF_IOREMAP in order to let this code calculate membase:

up->port.membase = ioremap(up->port.mapbase, size);

(drivers/serial/8250.c, function serial8250_request_std_resource)

but maybe we can just leave mapbase == 0 and change uart_match_port() as
in my patch.

Any other option?

Re: Preliminary patch for ip32 ttyS* device

[Giuseppe Sacco]

Il giorno mer, 31/10/2007 alle 13.08 +0000, Ralf Baechle ha scritto:
[...]
> > diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
> > index 3bb5d24..7caa877 100644
> > --- a/drivers/serial/serial_core.c
> > +++ b/drivers/serial/serial_core.c
> > @@ -2455,6 +2455,8 @@ int uart_match_port(struct uart_port *port1, struct uart_port *port2)
> >  	case UPIO_AU:
> >  	case UPIO_TSI:
> >  	case UPIO_DWAPB:
> > +		if (port1->mapbase==0 && port2->mapbase==0)
> > +			return (port1->membase == port2->membase);
> 
> This hack is only needed because ->mapbase is not initialized.

I have been investigating about it for one week and I am still not
convinced that mapbase must be initialised. I tried to understand the
meaning of mapbase and membase, but I am unsure about the value I should
set mapbase to.

I learnt that when specifying mapbase its region would be registered and
reserved using request_mem_region(). Otherwise, if you do not specify
mapbase, the region is not reserved. Apart from reserving the memory
region, mapbase isn't use anymore. Is mapbase mandatory? 

If mapbase isn't mandatory, the second part of my patch is probably
right and fixes a bug.

Bye,
Giuseppe

Re: Preliminary patch for ip32 ttyS* device

[Maciej W. Rozycki]

On Wed, 7 Nov 2007, Giuseppe Sacco wrote:

> I have been investigating about it for one week and I am still not
> convinced that mapbase must be initialised. I tried to understand the
> meaning of mapbase and membase, but I am unsure about the value I should
> set mapbase to.
> 
> I learnt that when specifying mapbase its region would be registered and
> reserved using request_mem_region(). Otherwise, if you do not specify
> mapbase, the region is not reserved. Apart from reserving the memory
> region, mapbase isn't use anymore. Is mapbase mandatory? 
> 
> If mapbase isn't mandatory, the second part of my patch is probably
> right and fixes a bug.

 You ought to use mapbase and ioremap() with new code as you are not 
allowed to use readb()/writeb()/etc. on addresses obtained otherwise than 
by calling ioremap().  The use of request_mem_region(), etc. is not 
strictly mandatory, but it is nice to have.  Many serial drivers use these 
functions, so I cannot see a reason why it would be a hassle for ip32.

  Maciej

Re: Preliminary patch for ip32 ttyS* device

[Martin Michlmayr]

* Maciej W. Rozycki <macro@linux-mips.org> [2007-11-07 17:21]:
> > If mapbase isn't mandatory, the second part of my patch is
> > probably right and fixes a bug.
> 
> You ought to use mapbase and ioremap() with new code as you are not
> allowed to use readb()/writeb()/etc. on addresses obtained otherwise
> than by calling ioremap().  The use of request_mem_region(), etc. is
> not strictly mandatory, but it is nice to have.  Many serial drivers
> use these functions, so I cannot see a reason why it would be a
> hassle for ip32.

Can someone propose a patch?  It's quote unfortuntely that serial on
IP32 is still broken (including 2.6.23).
-- 
Martin Michlmayr
http://www.cyrius.com/