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
--
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
