Re: [PATCH] 8250_early console support for au1x00 (again)

[Rodolfo Giometti <giometti@linux.it>]

>    Sorry for follwing up 2 month ago, I just happen to stumble on some 
>    issues addresses by these patches as well. I assume you haven't tried 
> sending them to Russel King?

Not yet, I just waiting for some comments. :)

> >+	switch (port->iotype) {
> >+	case UPIO_HUB6:
> >+		outb(port->hub6 - 1 + offset, port->iobase);
> >+		return inb(port->iobase + 1);
> 
>    This module can't handle HAB6 otherwise, so whay add it here?

Ok. I'm going to remove it.

> >+	case UPIO_MEM32:
> >+	case UPIO_AU:
> >+		return readl(port->membase + offset);
> 
>    NAK. readl() can't be used to read from Alchemy SOC peripherals because 
> it'll break in BE mode.  Alchemy automagically handles byteswap for the SOC 
> peripherals.

Ok. I'm going to fix it by using au_readl() but in this case I have to
add an ifdef with au1xxx include file. Can it be acceptable?

> >@@ -142,6 +266,7 @@ static int __init parse_options(struct e
> > 	port->uartclk = BASE_BAUD * 16;
> > 	if (!strncmp(options, "mmio,", 5)) {
> > 		port->iotype = UPIO_MEM;
> >+		port->regshift = 0;
> 
>    Maybe regshift even deserves the separate (but optional) field in the 
>    boot option...

Mmm... I don't know. However I have to set a default value.

> >+			(port->iotype == UPIO_MEM) ? "MMIO" : \
> >+			(port->iotype == UPIO_AU)  ? "AU"   : "I/O port",
> >+			(port->iotype == UPIO_MEM) || \
> >+			(port->iotype == UPIO_AU) ? port->mapbase :
> > 			    (unsigned long) port->iobase);
> 
>    I'd simply map UPIO_AU to "MMIO" in the message because it's memory 
>    mapped UART after all...

Yes, but in the kernel command line we must supply "au"... That's why
I used different string, so the user can verify whatever he/she passed
to the kernel.

> >index 17839e7..9e27aee 100644
> >--- a/drivers/serial/serial_core.c
> >+++ b/drivers/serial/serial_core.c
> >@@ -2367,6 +2367,7 @@ int uart_match_port(struct uart_port *po
> > 		return (port1->iobase == port2->iobase) &&
> > 		       (port1->hub6   == port2->hub6);
> > 	case UPIO_MEM:
> >+	case UPIO_AU:
> 
>    Also needs cases for UPIO_MEM32 and UPIO_TSI.

I just added the code for au1xxx. Why should I consider those cases
also?

> >-#ifdef CONFIG_SERIAL_8250_AU1X00
> > 	case UPIO_AU:
> >-		__raw_writel(value, up->port.membase + offset);
> >+		writel(value, up->port.membase + offset);
> > 		break;
> >-#endif
> 
>    Ditto writel().

Is __raw_writel() correct?

> WBR, Sergei

Thanks for your suggestions.

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127