Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753300AbbHCQea (ORCPT ); Mon, 3 Aug 2015 12:34:30 -0400 Received: from mail-qg0-f51.google.com ([209.85.192.51]:36530 "EHLO mail-qg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752783AbbHCQe2 (ORCPT ); Mon, 3 Aug 2015 12:34:28 -0400 Message-ID: <55BF980E.20908@hurleysoftware.com> Date: Mon, 03 Aug 2015 12:34:22 -0400 From: Peter Hurley User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Sebastian Andrzej Siewior CC: John Ogness , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Tony Lindgren , linux-omap@vger.kernel.org, nsekhar@ti.com, nm@ti.com, linux-serial@vger.kernel.org, Heikki Krogerus Subject: Re: [PATCH 3/3] serial: 8250: omap: restore registers on shutdown References: <87egjp2r4a.fsf@linutronix.de> <55BAC67E.1010400@hurleysoftware.com> <20150803160937.GA26497@linutronix.de> In-Reply-To: <20150803160937.GA26497@linutronix.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5692 Lines: 183 [ +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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/