Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932098AbbHCQzH (ORCPT ); Mon, 3 Aug 2015 12:55:07 -0400 Received: from www.linutronix.de ([62.245.132.108]:33473 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754578AbbHCQzD (ORCPT ); Mon, 3 Aug 2015 12:55:03 -0400 Message-ID: <55BF9CE0.4080107@linutronix.de> Date: Mon, 03 Aug 2015 18:54:56 +0200 From: Sebastian Andrzej Siewior User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: Peter Hurley 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> <55BF980E.20908@hurleysoftware.com> In-Reply-To: <55BF980E.20908@hurleysoftware.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5352 Lines: 159 On 08/03/2015 06:34 PM, Peter Hurley wrote: > Hi Sebastian, Hi Peter, >> 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? Not that I know of. You still need to ensure that once that DMA completed, nobody triggers another TX transfer before you do what you planned. This is ensures by the tx_running != 0 and the spin lock. > Maybe we could wrap that in the 8250 dma api? You mean a function in 8250-dma API which does what I did just here with the wait_event() and the wake_up in the callback? That way I could move the termios_wait into the dma struct instead of keeping in the omap specific part. I am also not sure if OMAP is the only one that may hang here or the other people just didn't notice it yet. > 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/