[PATCH] Xilinx UART Lite 2.6.18 driver

[David Bolcsfoldi]

On 10/27/06, Peter Korsgaard <jacmet at sunsite.dk> wrote:
> >>>>> "Peter" == Peter Korsgaard <jacmet at sunsite.dk> writes:
>
> Peter> I'll test and take a closer look at your patch when I have
> Peter> access to hw again on Monday.
>
> Sorry, it got a bit later than promised, but I've now had a closer
> look at your patch ..
>
> --- uartlite/arch/ppc/boot/simple/misc.c        2006-10-15 14:09:47.000000000 -0700
> +++ uartlite-mod/arch/ppc/boot/simple/misc.c    2006-10-15 13:58:51.000000000 -0700
> @@ -48,7 +48,8 @@
>  #if (defined(CONFIG_SERIAL_8250_CONSOLE) \
>         || defined(CONFIG_VGA_CONSOLE) \
>         || defined(CONFIG_SERIAL_MPC52xx_CONSOLE) \
> -       || defined(CONFIG_SERIAL_MPSC_CONSOLE)) \
> +       || defined(CONFIG_SERIAL_MPSC_CONSOLE) \
> +       || defined(CONFIG_SERIAL_UARTLITE_CONSOLE)) \
>         && !defined(CONFIG_GEMINI)
>  #define INTERACTIVE_CONSOLE    1
>  #endif
>
> The Xilinx boards use misc-embedded.c not misc.c - Why is this needed?
>

You are right, it's not.

> +unsigned long serial_init(int chan, void *ignored)
> +{
> +       switch (chan)  {
> +       #ifdef XPAR_XUL_UART_0_BASEADDR
> +               case 0:
> +                       return XPAR_XUL_UART_0_BASEADDR;
> +       #endif
> +       #ifdef XPAR_XUL_UART_1_BASEADDR
> +               case 1:
> +                       return XPAR_XUL_UART_1_BASEADDR;
> +       #endif
> +       #ifdef XPAR_XUL_UART_2_BASEADDR
> +               case 2:
> +                       return XPAR_XUL_UART_2_BASEADDR;
> +       #endif
> +       #ifdef XPAR_XUL_UART_3_BASEADDR
> +               case 3:
> +                       return XPAR_XUL_UART_3_BASEADDR;
> +       #endif
>
> This doesn't help much as you don't use the com_port argument in the
> other functions.
>

True, it's utterly useless. I was planning on having an array with
base addresses which would be used to support the com_port argument.
But it didn't make it in this patch although I have version that does
this now.

> Where did you get the XPAR_XUL_UART_ defines from? Our xparameters.h
> seem to contain XPAR_UARTLITE_ defines instead.
>

I think that name is picked up from the name of the device in your
design, the defines get names XPAR_NNN_ and mine are called XUL_UART.

> @@ -131,12 +115,16 @@
>         struct uart_port *port = (struct uart_port *)dev_id;
>         int busy;
>
> +       spin_lock(&port->lock); /* Lock the port in case of printk */
> +
>
> In an interrupt handler? Why? The console_write does a spin_lock_irqsave.
>

Sorry, my mistake. It's not needed.

>  static int __init ulite_console_setup(struct console *co, char *options)
>  {
> +       int i, ret;
>         struct uart_port *port;
> -
> +       struct platform_device *pdev;
> +
>         if (co->index < 0 || co->index >= ULITE_NR_UARTS)
>                 return -EINVAL;
>
>         port = &ports[co->index];
>
>         /* not initialized yet? */
> -       if (!port->membase)
> -               return -ENODEV;
> +       if (!port->membase) {
> +               /* We might be early console */
>
> Is this really necessary? The platform probe get's called quite early, E.G.:
>
> Breakpoint 2, ulite_probe (pdev=0xc01c84d0) at drivers/serial/uartlite.c:392
> 392     {
> (gdb) print __log_buf
> $1 = "<5>Linux version 2.6.19-rc3 (peko at sleipner) (gcc version 3.4.5) #19 Fri Oct 27 16:39:00 CEST 2006\n<6>Barco ThinLite (V2P) <peter.korsgaard at barco.com>\n<7>Entering add_active_range(0, 0, 15360) 0 entrie"...
>
> You can always use the ppc_md.progress() stuff for really early
> debugging if needed. I would prefer to keep this workaround out of
> uartlite.c if it isn't needed.
>

I am pretty certain probe won't get called until ppc_sys_init has been
called which is fairly far into the booting process and even if it did
the platform_bus hasn't been initialized before the ppc_sys_init is
called, at least not with any of the devices exported by
ppc_sys_devices.

Thanks for the review!
David


> --
> Bye, Peter Korsgaard
>