Re: [RFC][PATCH] Xilinx uartlite serial driver

Peter Korsgaard wrote:
> > > > > > "David" == David H Lynch <dhlii@xxxxxxxxxx> writes:
> > > > > >             
>
> Hi,
>
>  David>      Your ulite_console_write performance is pretty slow -
>  David> almost an order of magnitude slower than what I am seeing in
>  David> my driver. I beleive it is because you are waiting for the Tx
>  David> FIFO to be empty before and after sending each character
>  David> rather than only waiting while the Tx FIFO is full.
>
> Really? The implementation is basically a copy of 8250.c, so I would
> imagine that would get similar performance. Perhaps it's related to
> your specific setup?
>
> The driver only waits before sending a character, not afterwards.
>   

   From your code:
   there is a call to ulite_console_wait_tx() in each call to 
ulite_console_putchar() and a call after uart_console_write()
   However as I later determined that is NOT the problem. Either 
uart_console_write() is very slow, or there is a parameter wrong - 
probably in uart_port.
   But if you have something wrong there. So do I because when I switch 
my code to uase uart_console_write() instead of the a manual 
implimentation of puts (what older drivers use, including older 8250's)
   I get the same problem. I think my driver using uart_console_write() 
may be faster - but that may be wishful thinking. Both are abysmal 
compared to a simple puts loop. The difference is dramatic.

+#ifdef CONFIG_SERIAL_UARTLITE_CONSOLE
+static void ulite_console_wait_tx(struct uart_port *port)
+{
+       int i;
+
+       /* wait up to 10ms for the character(s) to be sent */
+       for (i=0; i<10000; i++) {
+               if (readb(port->membase + ULITE_STATUS) & ULITE_STATUS_TXEMPTY)
+                       break;
+               udelay(1);
+       }
+}
+
+static void ulite_console_putchar(struct uart_port *port, int ch)
+{
+       ulite_console_wait_tx(port);
+       writeb(ch, port->membase + ULITE_TX);
+}
+
+static void ulite_console_write(struct console *co, const char *s,
+                               unsigned int count)
+{
+       struct uart_port *port = &ports[co->index];
+       unsigned long flags;
+       unsigned int ier;
+       int locked = 1;
+
+       if (oops_in_progress) {
+               locked = spin_trylock_irqsave(&port->lock, flags);
+       } else
+               spin_lock_irqsave(&port->lock, flags);
+
+       /* save and disable interrupt */
+       ier = readb(port->membase + ULITE_STATUS) & ULITE_STATUS_IE;
+       writeb(0, port->membase + ULITE_CONTROL);
+
+       uart_console_write(port, s, count, ulite_console_putchar);
+
+       ulite_console_wait_tx(port);
+
+       /* restore interrupt state */
+       if (ier)
+               writeb(ULITE_CONTROL_IE, port->membase + ULITE_CONTROL);
+
+       if (locked)
+               spin_unlock_irqrestore(&port->lock, flags);
+}
+




>  David>      That may also be an issue in the rest of the driver but I have been
>  David> unable to get it to through registering the driver.
>  David>      I suspect that is because your driver is a platform driver, but 
> you did
>  David> not provide patches to the arch/ppc/platforms/4xx/virtex.* or
>  David> xilinx_ml???.* to setup the platform device structure.
>
> No, because the Xilinx boards don't have any uartlite devices.
>
>  David>      If you could provide a copy I would appreciate it.
>
> Sure, just add something like this to your board code (with your base
> address/irq ofcause):
>         
> static struct resource uartlite_resources[] = {
>        [0] = {
>            .start  = 0xa1000003,
>            .end    = 0xa1000012,
>            .flags  = IORESOURCE_MEM,
>        },
>        [1] = {
>            .start  = 2,
>            .end    = 2,
>            .flags  = IORESOURCE_IRQ,
>        },
> };
>
> static struct platform_device uartlite = {
>        .name              = "uartlite",
>        .id                = 0,
>        .num_resources     = ARRAY_SIZE(uartlite_resources),
>        .resource          = uartlite_resources,
>        .dev.platform_data = 0,
> };
>
> static struct platform_device *my_devices[] __initdata = {
>        &uartlite,
>        .
>        .
>        .
> };
>
> static int __init
> my_platform_add_devices(void)
> {
>         return platform_add_devices(my_devices, ARRAY_SIZE(my_devices));
>
> }
>
> arch_initcall(my_platform_add_devices);
>
>   


--
Dave Lynch                                                  DLA Systems
Software Development:                                    Embedded Linux
717.627.3770           dhlii@xxxxxxxxxx           http://www.dlasys.net
fax: 1.253.369.9244                                Cell: 1.717.587.7774
Over 25 years' experience in platforms, languages, and technologies too 
numerous to list.

"Any intelligent fool can make things bigger and more complex... It takes a touch of 
genius - and a lot of courage to move in the opposite direction."
Albert Einstein

-
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html