Re: [Nios2-dev] [PATCH 2/2] serial: Add driver for the Altera UART

[Tobias Klauser <klto@zhaw.ch>]

Hi Ian,

Thanks a lot for your review. Please see my notes below.

On 05/06/2010 12:44 PM, Ian Abbott wrote:
> On Wed, 2010-05-05 at 09:35 +0100, Tobias Klauser wrote:
>> Add an UART driver for the UART component available as a SOPC (System on
>> Programmable Chip) component for Altera FPGAs.
> [...]
> 
>> +static void altera_uart_start_tx(struct uart_port *port)
>> +{
>> +       struct altera_uart *pp = container_of(port, struct altera_uart, port);
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&port->lock, flags);
>> +       pp->imr |= ALTERA_UART_CONTROL_TRDY_MSK;
>> +       writel(pp->imr, port->membase + ALTERA_UART_CONTROL_REG);
>> +       spin_unlock_irqrestore(&port->lock, flags);
>> +}
>> +
>> +static void altera_uart_stop_tx(struct uart_port *port)
>> +{
>> +       struct altera_uart *pp = container_of(port, struct altera_uart, port);
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&port->lock, flags);
>> +       pp->imr &= ~ALTERA_UART_CONTROL_TRDY_MSK;
>> +       writel(pp->imr, port->membase + ALTERA_UART_CONTROL_REG);
>> +       spin_unlock_irqrestore(&port->lock, flags);
>> +}
>> +
>> +static void altera_uart_stop_rx(struct uart_port *port)
>> +{
>> +       struct altera_uart *pp = container_of(port, struct altera_uart, port);
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&port->lock, flags);
>> +       pp->imr &= ~ALTERA_UART_CONTROL_RRDY_MSK;
>> +       writel(pp->imr, port->membase + ALTERA_UART_CONTROL_REG);
>> +       spin_unlock_irqrestore(&port->lock, flags);
>> +}
> 
> Those four functions shouldn't grab port->lock because the caller in
> serial_core.c grabs it before calling them.

Agreed, I removed the lock grabbing from altera_uart_stop_rx,
altera_uart_stop_tx, altera_uart_start_tx, altera_uart_set_mctrl and
altera_uart_get_mctrl (as suggested in the original message and the two
followups).

I also noticed, that altera_uart_break_ctl takes the lock which might be
not necessary as the caller in serial_core.c already takes port->mutex
which should be enough. Or am I wrong?

>> +static irqreturn_t altera_uart_interrupt(int irq, void *data)
>> +{
>> +       struct uart_port *port = data;
>> +       struct altera_uart *pp = container_of(port, struct altera_uart, port);
>> +       unsigned int isr;
>> +
>> +       isr = readl(port->membase + ALTERA_UART_STATUS_REG) & pp->imr;
>> +       if (isr & ALTERA_UART_STATUS_RRDY_MSK)
>> +               altera_uart_rx_chars(pp);
>> +       if (isr & ALTERA_UART_STATUS_TRDY_MSK)
>> +               altera_uart_tx_chars(pp);
>> +       return IRQ_RETVAL(isr);
>> +}
> 
> Either the interrupt routine should grab port->lock (see equivalent
> function in altera_jtaguart.c for an example), or this could be done
> lower down in altera_uart_rx_chars() and altera_uart_tx_chars(). (It's
> less messing about to do it here in the interrupt routine though.)

I added the lock to the interrupt routine (similar to altera_jtaguart.c).

>> +static void altera_uart_console_putc(struct console *co, const char c)
>> +{
>> +       struct uart_port *port = &(altera_uart_ports + co->index)->port;
>> +       int i;
>> +
>> +       for (i = 0; i < 0x10000; i++) {
>> +               if (readl(port->membase + ALTERA_UART_STATUS_REG) &
>> +                   ALTERA_UART_STATUS_TRDY_MSK)
>> +                       break;
>> +       }
>> +       writel(c, port->membase + ALTERA_UART_TXDATA_REG);
>> +       for (i = 0; i < 0x10000; i++) {
>> +               if (readl(port->membase + ALTERA_UART_STATUS_REG) &
>> +                   ALTERA_UART_STATUS_TRDY_MSK)
>> +                       break;
>> +       }
>> +}
> 
> I'd suggest calling udelay(1) in each iteration of the loop.  Perhaps
> the second loop is overkill.

Those seem to come from mcf.c on which this driver was based. I added
the udelay to the first loop and dropped the second loop which indeed
isn't needed here. I also replace the first loop with the equivalent
construct from altera_uart_tx_chars.

> I was also going to suggest using the port->lock spin lock, but I'm not
> sure about the policy regarding that for console output (the 8250 driver
> console output doesn't use the spin lock for example).

As I understood from Alan's comment on altera_jtaguart.c the lock is
needed to protect port->port.tty so I'll add it in altera_uart.c too.

Cheers,
Tobias