[ +cc Heikki ]
Hi Sebastian,
On 08/03/2015 12:09 PM, Sebastian Andrzej Siewior wrote:
> * Peter Hurley | 2015-07-30 20:51:10 [-0400]:
>
>> Hi John,
> Hi Peter,
>
>> I was never really a fan of the deferred set_termios();
>> I think it's more appropriate to wait for tx dma to
>> complete in omap_8250_set_termios().
>
> So you want something like this? This was only compile + boot tested
> (without triggering the corner case) and I know that 8250.h piece has to
> go in a separated patch (as requested in 2/3 of this series). Just checking
> if this is what you had in mind.
>
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index c43f74c53cd9..a407757dcecc 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -42,9 +42,9 @@ struct uart_8250_dma {
> size_t rx_size;
> size_t tx_size;
>
> - unsigned char tx_running:1;
> - unsigned char tx_err: 1;
> - unsigned char rx_running:1;
> + unsigned char tx_running;
> + unsigned char tx_err;
> + unsigned char rx_running;
> };
This part is ok.
> struct old_serial_port {
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index d9a37191a1ae..12249125a218 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -100,9 +100,9 @@ struct omap8250_priv {
> u8 wer;
> u8 xon;
> u8 xoff;
> - u8 delayed_restore;
> u16 quot;
>
> + wait_queue_head_t termios_wait;
> bool is_suspending;
> int wakeirq;
> int wakeups_enabled;
> @@ -256,18 +256,6 @@ static void omap8250_update_mdr1(struct uart_8250_port *up,
> static void omap8250_restore_regs(struct uart_8250_port *up)
> {
> struct omap8250_priv *priv = up->port.private_data;
> - struct uart_8250_dma *dma = up->dma;
> -
> - if (dma && dma->tx_running) {
> - /*
> - * TCSANOW requests the change to occur immediately however if
> - * we have a TX-DMA operation in progress then it has been
> - * observed that it might stall and never complete. Therefore we
> - * delay DMA completes to prevent this hang from happen.
> - */
> - priv->delayed_restore = 1;
> - return;
> - }
>
> serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> serial_out(up, UART_EFR, UART_EFR_ECB);
> @@ -309,6 +297,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
> up->port.ops->set_mctrl(&up->port, up->port.mctrl);
> }
>
> +static void omap_8250_dma_tx_complete(void *param);
> /*
> * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
> * some differences in how we want to handle flow control.
> @@ -322,6 +311,7 @@ static void omap_8250_set_termios(struct uart_port *port,
> struct omap8250_priv *priv = up->port.private_data;
> unsigned char cval = 0;
> unsigned int baud;
> + unsigned int complete_dma = 0;
>
> switch (termios->c_cflag & CSIZE) {
> case CS5:
> @@ -473,6 +463,25 @@ static void omap_8250_set_termios(struct uart_port *port,
> if (termios->c_iflag & IXANY)
> up->mcr |= UART_MCR_XONANY;
> }
> +
> + if (up->dma && up->dma->tx_running) {
> + struct uart_8250_dma *dma = up->dma;
> +
> + /*
> + * TCSANOW requests the change to occur immediately however if
> + * we have a TX-DMA operation in progress then it has been
> + * observed that it might stall and never complete. Therefore we
> + * wait until DMA completes to prevent this hang from happen.
> + */
> +
> + dma->tx_running = 2;
> +
> + spin_unlock_irq(&up->port.lock);
> + wait_event(priv->termios_wait,
> + dma->tx_running == 3);
Doesn't the dmaengine api offer a race-free way to wait for pending tx dma
to complete?
Maybe we could wrap that in the 8250 dma api?
Regards,
Peter Hurley
> + spin_lock_irq(&up->port.lock);
> + complete_dma = 1;
> + }
> omap8250_restore_regs(up);
>
> spin_unlock_irq(&up->port.lock);
> @@ -488,6 +497,8 @@ static void omap_8250_set_termios(struct uart_port *port,
> /* Don't rewrite B0 */
> if (tty_termios_baud_rate(termios))
> tty_termios_encode_baud_rate(termios, baud, baud);
> + if (complete_dma)
> + omap_8250_dma_tx_complete(up);
> }
>
> /* same as 8250 except that we may have extra flow bits set in EFR */
> @@ -869,17 +880,18 @@ static void omap_8250_dma_tx_complete(void *param)
>
> spin_lock_irqsave(&p->port.lock, flags);
>
> + if (dma->tx_running == 2) {
> + dma->tx_running = 3;
> + wake_up(&priv->termios_wait);
> + goto out;
> + }
> +
> dma->tx_running = 0;
>
> xmit->tail += dma->tx_size;
> xmit->tail &= UART_XMIT_SIZE - 1;
> p->port.icount.tx += dma->tx_size;
>
> - if (priv->delayed_restore) {
> - priv->delayed_restore = 0;
> - omap8250_restore_regs(p);
> - }
> -
> if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> uart_write_wakeup(&p->port);
>
> @@ -899,7 +911,7 @@ static void omap_8250_dma_tx_complete(void *param)
> p->ier |= UART_IER_THRI;
> serial_port_out(&p->port, UART_IER, p->ier);
> }
> -
> +out:
> spin_unlock_irqrestore(&p->port.lock, flags);
> }
>
> @@ -1216,6 +1228,7 @@ static int omap8250_probe(struct platform_device *pdev)
> priv->omap8250_dma.rx_size = RX_TRIGGER;
> priv->omap8250_dma.rxconf.src_maxburst = RX_TRIGGER;
> priv->omap8250_dma.txconf.dst_maxburst = TX_TRIGGER;
> + init_waitqueue_head(&priv->termios_wait);
>
> if (of_machine_is_compatible("ti,am33xx"))
> priv->habit |= OMAP_DMA_TX_KICK;
>
>
> Sebastian
>
--
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
