dma support for 8250-omap serial driver

dma support for 8250-omap serial driver

[Sebastian Andrzej Siewior]

Hi,

this series is on top of "[PATCH v4] 8250-core based serial driver for OMAP".
I will refresh it once I fixed the runtime-pm issue.
Patches 1-3 are independent of the series and just bug fixes. Patches 4+ are
RFC only for now. Its been tested on dra7, am33xx won't work as-is (even with
patch 5 providing dts nodes).

The complete series ontop of of v3.16-rc5 is also available at
	git://git.breakpoint.cc/bigeasy/linux.git uart_v5_dma

Sebastian

dma support for 8250-omap serial driver

[Sebastian Andrzej Siewior]

Hi,

this series is on top of "[PATCH v4] 8250-core based serial driver for OMAP".
I will refresh it once I fixed the runtime-pm issue.
Patches 1-3 are independent of the series and just bug fixes. Patches 4+ are
RFC only for now. Its been tested on dra7, am33xx won't work as-is (even with
patch 5 providing dts nodes).

The complete series ontop of of v3.16-rc5 is also available at
	git://git.breakpoint.cc/bigeasy/linux.git uart_v5_dma

Sebastian

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

The rx path of the 8250_dma user in the RX-timeout case:
- it starts the RX transfer
- if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
- step two is dmaengine_terminate_all() on this channel.
- based on dmaengine_tx_status() it learns the number of transfered
  bytes.
- the rx interrupt occures, it does not start the RX-transfer because
  according to dmaengine_tx_status() the status of the current transfer
  is still DMA_IN_PROGRESS because the earlier dmaengine_terminate_all()
  did not reset this.
- on rx-timeout it invokes dmaengine_pause() again. This time, it
  segfaults because the channel has no descriptor yet.

To make the upper case work better, this patch adds dma_cookie_complete()
to complete the cookie. Also it adds is an additional check for echan->edesc
in case the channel has no descriptor assigned.

Cc: Joel Fernandes <joelf@ti.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: dmaengine@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/dma/edma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index d08c4de..ff05dd4 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -256,6 +256,7 @@ static int edma_terminate_all(struct edma_chan *echan)
 	 * echan->edesc is NULL and exit.)
 	 */
 	if (echan->edesc) {
+		dma_cookie_complete(&echan->edesc->vdesc.tx);
 		echan->edesc = NULL;
 		edma_stop(echan->ch_num);
 	}
@@ -282,7 +283,7 @@ static int edma_slave_config(struct edma_chan *echan,
 static int edma_dma_pause(struct edma_chan *echan)
 {
 	/* Pause/Resume only allowed with cyclic mode */
-	if (!echan->edesc->cyclic)
+	if (!echan->edesc || !echan->edesc->cyclic)
 		return -EINVAL;
 
 	edma_pause(echan->ch_num);
-- 
2.0.1

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

The rx path of the 8250_dma user in the RX-timeout case:
- it starts the RX transfer
- if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
- step two is dmaengine_terminate_all() on this channel.
- based on dmaengine_tx_status() it learns the number of transfered
  bytes.
- the rx interrupt occures, it does not start the RX-transfer because
  according to dmaengine_tx_status() the status of the current transfer
  is still DMA_IN_PROGRESS because the earlier dmaengine_terminate_all()
  did not reset this.
- on rx-timeout it invokes dmaengine_pause() again. This time, it
  segfaults because the channel has no descriptor yet.

To make the upper case work better, this patch adds dma_cookie_complete()
to complete the cookie. Also it adds is an additional check for echan->edesc
in case the channel has no descriptor assigned.

Cc: Joel Fernandes <joelf@ti.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: dmaengine at vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/dma/edma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index d08c4de..ff05dd4 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -256,6 +256,7 @@ static int edma_terminate_all(struct edma_chan *echan)
 	 * echan->edesc is NULL and exit.)
 	 */
 	if (echan->edesc) {
+		dma_cookie_complete(&echan->edesc->vdesc.tx);
 		echan->edesc = NULL;
 		edma_stop(echan->ch_num);
 	}
@@ -282,7 +283,7 @@ static int edma_slave_config(struct edma_chan *echan,
 static int edma_dma_pause(struct edma_chan *echan)
 {
 	/* Pause/Resume only allowed with cyclic mode */
-	if (!echan->edesc->cyclic)
+	if (!echan->edesc || !echan->edesc->cyclic)
 		return -EINVAL;
 
 	edma_pause(echan->ch_num);
-- 
2.0.1

[PATCH 2/7] dmaengine: omap-dma: complete the transfer on terminate_all

[Sebastian Andrzej Siewior]

The rx path of the 8250_dma user in the RX-timeout case:
- it starts the RX transfer
- if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
- step two is dmaengine_terminate_all() on this channel.
- the rx interrupt occures, it does not start the RX-transfer because
  according to dmaengine_tx_status() the status of the current transfer
  is still DMA_IN_PROGRESS because the earlier dmaengine_terminate_all()
  did not reset this.

To fix this problem, the patch adds dma_cookie_complete() "complete" the
current cookie.

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Vinod Koul <vinod.koul@intel.com> 
Cc: Dan Williams <dan.j.williams@intel.com> 
Cc: dmaengine@vger.kernel.org 
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/dma/omap-dma.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index b19f04f..81ede01 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -970,6 +970,12 @@ static int omap_dma_terminate_all(struct omap_chan *c)
 
 	/* Prevent this channel being scheduled */
 	spin_lock(&d->lock);
+	if (!c->desc && !list_empty(&c->node)) {
+		struct virt_dma_desc *vd = vchan_next_desc(&c->vc);
+
+		if (vd)
+			dma_cookie_complete(&vd->tx);
+	}
 	list_del_init(&c->node);
 	spin_unlock(&d->lock);
 
@@ -979,6 +985,7 @@ static int omap_dma_terminate_all(struct omap_chan *c)
 	 * c->desc is NULL and exit.)
 	 */
 	if (c->desc) {
+		dma_cookie_complete(&c->desc->vd.tx);
 		c->desc = NULL;
 		/* Avoid stopping the dma twice */
 		if (!c->paused)
-- 
2.0.1

[PATCH 2/7] dmaengine: omap-dma: complete the transfer on terminate_all

[Sebastian Andrzej Siewior]

The rx path of the 8250_dma user in the RX-timeout case:
- it starts the RX transfer
- if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
- step two is dmaengine_terminate_all() on this channel.
- the rx interrupt occures, it does not start the RX-transfer because
  according to dmaengine_tx_status() the status of the current transfer
  is still DMA_IN_PROGRESS because the earlier dmaengine_terminate_all()
  did not reset this.

To fix this problem, the patch adds dma_cookie_complete() "complete" the
current cookie.

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Vinod Koul <vinod.koul@intel.com> 
Cc: Dan Williams <dan.j.williams@intel.com> 
Cc: dmaengine at vger.kernel.org 
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/dma/omap-dma.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
index b19f04f..81ede01 100644
--- a/drivers/dma/omap-dma.c
+++ b/drivers/dma/omap-dma.c
@@ -970,6 +970,12 @@ static int omap_dma_terminate_all(struct omap_chan *c)
 
 	/* Prevent this channel being scheduled */
 	spin_lock(&d->lock);
+	if (!c->desc && !list_empty(&c->node)) {
+		struct virt_dma_desc *vd = vchan_next_desc(&c->vc);
+
+		if (vd)
+			dma_cookie_complete(&vd->tx);
+	}
 	list_del_init(&c->node);
 	spin_unlock(&d->lock);
 
@@ -979,6 +985,7 @@ static int omap_dma_terminate_all(struct omap_chan *c)
 	 * c->desc is NULL and exit.)
 	 */
 	if (c->desc) {
+		dma_cookie_complete(&c->desc->vd.tx);
 		c->desc = NULL;
 		/* Avoid stopping the dma twice */
 		if (!c->paused)
-- 
2.0.1

[PATCH 3/7] serial: 8250_dma: continue TX dma on dma error

[Sebastian Andrzej Siewior]

After TX dma completes the code will queue another DMA transfer. If
serial8250_tx_dma() fails then there is no plan B to continue the
transfer manually.
This patch enables the TX-fifo empty event so it can be tried again via
DMA or use the manual fallback in case it fails.

Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_dma.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 148ffe4..f9ef69f 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -36,8 +36,15 @@ static void __dma_tx_complete(void *param)
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&p->port);
 
-	if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port))
-		serial8250_tx_dma(p);
+	if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port)) {
+		int ret;
+
+		ret = serial8250_tx_dma(p);
+		if (ret && !(p->ier & UART_IER_THRI)) {
+			p->ier |= UART_IER_THRI;
+			serial_port_out(&p->port, UART_IER, p->ier);
+		}
+	}
 
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
-- 
2.0.1

[PATCH 3/7] serial: 8250_dma: continue TX dma on dma error

[Sebastian Andrzej Siewior]

After TX dma completes the code will queue another DMA transfer. If
serial8250_tx_dma() fails then there is no plan B to continue the
transfer manually.
This patch enables the TX-fifo empty event so it can be tried again via
DMA or use the manual fallback in case it fails.

Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_dma.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 148ffe4..f9ef69f 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -36,8 +36,15 @@ static void __dma_tx_complete(void *param)
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&p->port);
 
-	if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port))
-		serial8250_tx_dma(p);
+	if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port)) {
+		int ret;
+
+		ret = serial8250_tx_dma(p);
+		if (ret && !(p->ier & UART_IER_THRI)) {
+			p->ier |= UART_IER_THRI;
+			serial_port_out(&p->port, UART_IER, p->ier);
+		}
+	}
 
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
-- 
2.0.1

[PATCH 3/7] serial: 8250_dma: continue TX dma on dma error

[Sebastian Andrzej Siewior]

After TX dma completes the code will queue another DMA transfer. If
serial8250_tx_dma() fails then there is no plan B to continue the
transfer manually.
This patch enables the TX-fifo empty event so it can be tried again via
DMA or use the manual fallback in case it fails.

Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_dma.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 148ffe4..f9ef69f 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -36,8 +36,15 @@ static void __dma_tx_complete(void *param)
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&p->port);
 
-	if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port))
-		serial8250_tx_dma(p);
+	if (!uart_circ_empty(xmit) && !uart_tx_stopped(&p->port)) {
+		int ret;
+
+		ret = serial8250_tx_dma(p);
+		if (ret && !(p->ier & UART_IER_THRI)) {
+			p->ier |= UART_IER_THRI;
+			serial_port_out(&p->port, UART_IER, p->ier);
+		}
+	}
 
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
-- 
2.0.1

[RFC PATCH 4/7] serial: 8250_dma: enqueue RX dma again on completion.

[Sebastian Andrzej Siewior]

The omap needs a DMA request pending right away. If it is enqueued once
the bytes are in the FIFO then nothing will happen and the FIFO will be
later purged via RX-timeout interrupt.
This patch requeues RX-DMA request on completion but not if it was
canceled due to the RX-timeout.

Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c |  3 +++
 drivers/tty/serial/8250/8250_dma.c  | 12 +++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 7a33c30..098ff02 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1556,6 +1556,9 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 
 		if (!up->dma || dma_err)
 			status = serial8250_rx_chars(up, status);
+
+		if (dma_err == -ETIMEDOUT && port->type == PORT_OMAP_16750)
+			serial8250_rx_dma(up, 0);
 	}
 	serial8250_modem_status(up);
 	if (!up->dma && (status & UART_LSR_THRE))
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index f9ef69f..acb0a87 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -49,9 +49,8 @@ static void __dma_tx_complete(void *param)
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
 
-static void __dma_rx_complete(void *param)
+static void __dma_rx_do_complete(struct uart_8250_port *p, bool timeout)
 {
-	struct uart_8250_port	*p = param;
 	struct uart_8250_dma	*dma = p->dma;
 	struct tty_port		*tty_port = &p->port.state->port;
 	struct dma_tx_state	state;
@@ -67,10 +66,17 @@ static void __dma_rx_complete(void *param)
 
 	tty_insert_flip_string(tty_port, dma->rx_buf, count);
 	p->port.icount.rx += count;
+	if (!timeout && p->port.type == PORT_OMAP_16750)
+		serial8250_rx_dma(p, 0);
 
 	tty_flip_buffer_push(tty_port);
 }
 
+static void __dma_rx_complete(void *param)
+{
+	__dma_rx_do_complete(param, false);
+}
+
 int serial8250_tx_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -126,7 +132,7 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 		 */
 		if (dma_status == DMA_IN_PROGRESS) {
 			dmaengine_pause(dma->rxchan);
-			__dma_rx_complete(p);
+			__dma_rx_do_complete(p, true);
 		}
 		return -ETIMEDOUT;
 	default:
-- 
2.0.1

[RFC PATCH 4/7] serial: 8250_dma: enqueue RX dma again on completion.

[Sebastian Andrzej Siewior]

The omap needs a DMA request pending right away. If it is enqueued once
the bytes are in the FIFO then nothing will happen and the FIFO will be
later purged via RX-timeout interrupt.
This patch requeues RX-DMA request on completion but not if it was
canceled due to the RX-timeout.

Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c |  3 +++
 drivers/tty/serial/8250/8250_dma.c  | 12 +++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 7a33c30..098ff02 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1556,6 +1556,9 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 
 		if (!up->dma || dma_err)
 			status = serial8250_rx_chars(up, status);
+
+		if (dma_err == -ETIMEDOUT && port->type == PORT_OMAP_16750)
+			serial8250_rx_dma(up, 0);
 	}
 	serial8250_modem_status(up);
 	if (!up->dma && (status & UART_LSR_THRE))
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index f9ef69f..acb0a87 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -49,9 +49,8 @@ static void __dma_tx_complete(void *param)
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
 
-static void __dma_rx_complete(void *param)
+static void __dma_rx_do_complete(struct uart_8250_port *p, bool timeout)
 {
-	struct uart_8250_port	*p = param;
 	struct uart_8250_dma	*dma = p->dma;
 	struct tty_port		*tty_port = &p->port.state->port;
 	struct dma_tx_state	state;
@@ -67,10 +66,17 @@ static void __dma_rx_complete(void *param)
 
 	tty_insert_flip_string(tty_port, dma->rx_buf, count);
 	p->port.icount.rx += count;
+	if (!timeout && p->port.type == PORT_OMAP_16750)
+		serial8250_rx_dma(p, 0);
 
 	tty_flip_buffer_push(tty_port);
 }
 
+static void __dma_rx_complete(void *param)
+{
+	__dma_rx_do_complete(param, false);
+}
+
 int serial8250_tx_dma(struct uart_8250_port *p)
 {
 	struct uart_8250_dma		*dma = p->dma;
@@ -126,7 +132,7 @@ int serial8250_rx_dma(struct uart_8250_port *p, unsigned int iir)
 		 */
 		if (dma_status == DMA_IN_PROGRESS) {
 			dmaengine_pause(dma->rxchan);
-			__dma_rx_complete(p);
+			__dma_rx_do_complete(p, true);
 		}
 		return -ETIMEDOUT;
 	default:
-- 
2.0.1

[RFC PATCH 5/7] arm: dts: am33xx: add DMA properties for UART

[Sebastian Andrzej Siewior]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/boot/dts/am33xx.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 4a4e02d..cdccbd6 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -200,6 +200,8 @@
 			reg = <0x44e09000 0x2000>;
 			interrupts = <72>;
 			status = "disabled";
+			dmas = <&edma 26>, <&edma 27>;
+			dma-names = "tx", "rx";
 		};
 
 		uart1: serial@48022000 {
@@ -209,6 +211,8 @@
 			reg = <0x48022000 0x2000>;
 			interrupts = <73>;
 			status = "disabled";
+			dmas = <&edma 28>, <&edma 29>;
+			dma-names = "tx", "rx";
 		};
 
 		uart2: serial@48024000 {
@@ -218,6 +222,8 @@
 			reg = <0x48024000 0x2000>;
 			interrupts = <74>;
 			status = "disabled";
+			dmas = <&edma 30>, <&edma 31>;
+			dma-names = "tx", "rx";
 		};
 
 		uart3: serial@481a6000 {
-- 
2.0.1

[RFC PATCH 5/7] arm: dts: am33xx: add DMA properties for UART

[Sebastian Andrzej Siewior]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/boot/dts/am33xx.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 4a4e02d..cdccbd6 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -200,6 +200,8 @@
 			reg = <0x44e09000 0x2000>;
 			interrupts = <72>;
 			status = "disabled";
+			dmas = <&edma 26>, <&edma 27>;
+			dma-names = "tx", "rx";
 		};
 
 		uart1: serial@48022000 {
@@ -209,6 +211,8 @@
 			reg = <0x48022000 0x2000>;
 			interrupts = <73>;
 			status = "disabled";
+			dmas = <&edma 28>, <&edma 29>;
+			dma-names = "tx", "rx";
 		};
 
 		uart2: serial@48024000 {
@@ -218,6 +222,8 @@
 			reg = <0x48024000 0x2000>;
 			interrupts = <74>;
 			status = "disabled";
+			dmas = <&edma 30>, <&edma 31>;
+			dma-names = "tx", "rx";
 		};
 
 		uart3: serial@481a6000 {
-- 
2.0.1

[RFC PATCH 5/7] arm: dts: am33xx: add DMA properties for UART

[Sebastian Andrzej Siewior]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/boot/dts/am33xx.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 4a4e02d..cdccbd6 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -200,6 +200,8 @@
 			reg = <0x44e09000 0x2000>;
 			interrupts = <72>;
 			status = "disabled";
+			dmas = <&edma 26>, <&edma 27>;
+			dma-names = "tx", "rx";
 		};
 
 		uart1: serial at 48022000 {
@@ -209,6 +211,8 @@
 			reg = <0x48022000 0x2000>;
 			interrupts = <73>;
 			status = "disabled";
+			dmas = <&edma 28>, <&edma 29>;
+			dma-names = "tx", "rx";
 		};
 
 		uart2: serial at 48024000 {
@@ -218,6 +222,8 @@
 			reg = <0x48024000 0x2000>;
 			interrupts = <74>;
 			status = "disabled";
+			dmas = <&edma 30>, <&edma 31>;
+			dma-names = "tx", "rx";
 		};
 
 		uart3: serial at 481a6000 {
-- 
2.0.1

[RFC PATCH 6/7] arm: dts: dra7: add DMA properties for UART

[Sebastian Andrzej Siewior]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/boot/dts/dra7.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 10066f4..a904561 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -259,6 +259,8 @@
 			ti,hwmods = "uart1";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 49>, <&sdma 50>;
+			dma-names = "tx", "rx";
 		};
 
 		uart2: serial@4806c000 {
@@ -268,6 +270,8 @@
 			ti,hwmods = "uart2";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 51>, <&sdma 52>;
+			dma-names = "tx", "rx";
 		};
 
 		uart3: serial@48020000 {
@@ -277,6 +281,8 @@
 			ti,hwmods = "uart3";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 53>, <&sdma 54>;
+			dma-names = "tx", "rx";
 		};
 
 		uart4: serial@4806e000 {
@@ -286,6 +292,8 @@
 			ti,hwmods = "uart4";
 			clock-frequency = <48000000>;
                         status = "disabled";
+			dmas = <&sdma 55>, <&sdma 56>;
+			dma-names = "tx", "rx";
 		};
 
 		uart5: serial@48066000 {
@@ -295,6 +303,8 @@
 			ti,hwmods = "uart5";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 63>, <&sdma 64>;
+			dma-names = "tx", "rx";
 		};
 
 		uart6: serial@48068000 {
@@ -304,6 +314,8 @@
 			ti,hwmods = "uart6";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 79>, <&sdma 80>;
+			dma-names = "tx", "rx";
 		};
 
 		uart7: serial@48420000 {
-- 
2.0.1

[RFC PATCH 6/7] arm: dts: dra7: add DMA properties for UART

[Sebastian Andrzej Siewior]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/boot/dts/dra7.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 10066f4..a904561 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -259,6 +259,8 @@
 			ti,hwmods = "uart1";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 49>, <&sdma 50>;
+			dma-names = "tx", "rx";
 		};
 
 		uart2: serial@4806c000 {
@@ -268,6 +270,8 @@
 			ti,hwmods = "uart2";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 51>, <&sdma 52>;
+			dma-names = "tx", "rx";
 		};
 
 		uart3: serial@48020000 {
@@ -277,6 +281,8 @@
 			ti,hwmods = "uart3";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 53>, <&sdma 54>;
+			dma-names = "tx", "rx";
 		};
 
 		uart4: serial@4806e000 {
@@ -286,6 +292,8 @@
 			ti,hwmods = "uart4";
 			clock-frequency = <48000000>;
                         status = "disabled";
+			dmas = <&sdma 55>, <&sdma 56>;
+			dma-names = "tx", "rx";
 		};
 
 		uart5: serial@48066000 {
@@ -295,6 +303,8 @@
 			ti,hwmods = "uart5";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 63>, <&sdma 64>;
+			dma-names = "tx", "rx";
 		};
 
 		uart6: serial@48068000 {
@@ -304,6 +314,8 @@
 			ti,hwmods = "uart6";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 79>, <&sdma 80>;
+			dma-names = "tx", "rx";
 		};
 
 		uart7: serial@48420000 {
-- 
2.0.1

[RFC PATCH 6/7] arm: dts: dra7: add DMA properties for UART

[Sebastian Andrzej Siewior]

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm/boot/dts/dra7.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 10066f4..a904561 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -259,6 +259,8 @@
 			ti,hwmods = "uart1";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 49>, <&sdma 50>;
+			dma-names = "tx", "rx";
 		};
 
 		uart2: serial at 4806c000 {
@@ -268,6 +270,8 @@
 			ti,hwmods = "uart2";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 51>, <&sdma 52>;
+			dma-names = "tx", "rx";
 		};
 
 		uart3: serial at 48020000 {
@@ -277,6 +281,8 @@
 			ti,hwmods = "uart3";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 53>, <&sdma 54>;
+			dma-names = "tx", "rx";
 		};
 
 		uart4: serial at 4806e000 {
@@ -286,6 +292,8 @@
 			ti,hwmods = "uart4";
 			clock-frequency = <48000000>;
                         status = "disabled";
+			dmas = <&sdma 55>, <&sdma 56>;
+			dma-names = "tx", "rx";
 		};
 
 		uart5: serial at 48066000 {
@@ -295,6 +303,8 @@
 			ti,hwmods = "uart5";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 63>, <&sdma 64>;
+			dma-names = "tx", "rx";
 		};
 
 		uart6: serial at 48068000 {
@@ -304,6 +314,8 @@
 			ti,hwmods = "uart6";
 			clock-frequency = <48000000>;
 			status = "disabled";
+			dmas = <&sdma 79>, <&sdma 80>;
+			dma-names = "tx", "rx";
 		};
 
 		uart7: serial at 48420000 {
-- 
2.0.1

[RFC PATCH 7/7] tty: serial: 8250: omap: add dma support

[Sebastian Andrzej Siewior]

This patch adds the required pieces to OMAP uart for DMA support. The TX
burst size is set to 1 so we can send an arbitrary amout of bytes.

The RX burst is currently set to 8 which means we receive an interrupt
every eight bytes and have to reprogramm everything. Seven bytes in the
RX-fifo mean that no transfer will happen and the UART will send a
RX-timeout event at which point the FIFO will be manually purged.
I played around with the trigger value and burst and burst of one, trigger
eight isn't really better. We receive a DMA-complete event once we
received eight. Should we receive only 7 bytes then the DMA will fetch
them and wait for the last one and there will be RX-timeout from the
UART because the RX-FIFO is empty.
Since this makes most sense for high baudrates I think I will increment
the RX size to something larger.
This works fine on DRA7. AM33xx is a diffent story and I run into two
problems:
- TX, after ptogramming the TX transfer there has to be a byte written
  into the FIFO to trigger the transfer. After that, the transfer
  continues and reloads as expected. I had a workaround for this but
  wasn't working perfectly.
- RX, after the DMA engines fetches the data from the FIFO there is not
  only a dma-complete interrupt but also an UART interrupt which
  reports nothing (because the FIFO is already empty). I managed a few
  to receive wrong data if the UART interrupt was delayed and I am not
  yet sure if this is a real problem, a theoretical one or just a bug
  somewhere in the driver(s)

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 92 +++++++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 4d01f45..6cc8174 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -20,6 +20,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/console.h>
 #include <linux/pm_qos.h>
+#include <linux/edma.h>
 
 #include "8250.h"
 
@@ -31,10 +32,16 @@
 #define UART_ERRATA_i202_MDR1_ACCESS	(1 << 0)
 #define OMAP_UART_WER_HAS_TX_WAKEUP	(1 << 1)
 
+#define OMAP_UART_FCR_RX_TRIG		6
+#define OMAP_UART_FCR_TX_TRIG		4
+
 /* SCR register bitmasks */
 #define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK	(1 << 7)
 #define OMAP_UART_SCR_TX_TRIG_GRANU1_MASK	(1 << 6)
 #define OMAP_UART_SCR_TX_EMPTY			(1 << 3)
+#define OMAP_UART_SCR_DMAMODE_MASK		(3 << 1)
+#define OMAP_UART_SCR_DMAMODE_1			(1 << 1)
+#define OMAP_UART_SCR_DMAMODE_CTL		(1 << 0)
 
 /* MVR register bitmasks */
 #define OMAP_UART_MVR_SCHEME_SHIFT	30
@@ -45,6 +52,12 @@
 #define OMAP_UART_MVR_MAJ_SHIFT		8
 #define OMAP_UART_MVR_MIN_MASK		0x3f
 
+#define UART_TI752_TLR_TX	0
+#define UART_TI752_TLR_RX	4
+
+#define TRIGGER_TLR_MASK(x)	((x & 0x3c) >> 2)
+#define TRIGGER_FCR_MASK(x)	(x & 3)
+
 /* Enable XON/XOFF flow control on output */
 #define OMAP_UART_SW_TX		0x08
 /* Enable XON/XOFF flow control on input */
@@ -78,6 +91,7 @@ struct omap8250_priv {
 	u32 calc_latency;
 	struct pm_qos_request pm_qos_request;
 	struct work_struct qos_work;
+	struct uart_8250_dma omap8250_dma;
 };
 
 static u32 uart_read(struct uart_8250_port *up, u32 reg)
@@ -158,6 +172,20 @@ static void omap_8250_get_divisor(struct uart_port *port, unsigned int baud,
 	}
 }
 
+static void omap8250_update_scr(struct uart_8250_port *up,
+		struct omap8250_priv *priv)
+{
+	/*
+	 * As it turns out the DMA part in the UART strikes if the CONTROL bit
+	 * and the DMA-MODE is set at the same time. Therefore we first set the
+	 * control bit with DMA off and then we switch to the required DMA mode.
+	 */
+	if (priv->scr & OMAP_UART_SCR_DMAMODE_MASK)
+		serial_out(up, UART_OMAP_SCR,
+				priv->scr & ~OMAP_UART_SCR_DMAMODE_MASK);
+	serial_out(up, UART_OMAP_SCR, priv->scr);
+}
+
 /*
  * 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.
@@ -273,7 +301,19 @@ static void omap_8250_set_termios(struct uart_port *port,
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 
-	priv->scr = OMAP_UART_SCR_RX_TRIG_GRANU1_MASK | OMAP_UART_SCR_TX_EMPTY;
+	serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
+
+	serial_out(up, UART_TI752_TLR,
+			TRIGGER_TLR_MASK(1) << UART_TI752_TLR_TX |
+			TRIGGER_TLR_MASK(8) << UART_TI752_TLR_RX);
+
+	priv->scr =  OMAP_UART_SCR_RX_TRIG_GRANU1_MASK | OMAP_UART_SCR_TX_EMPTY;
+	if (up->dma) {
+		priv->scr |= OMAP_UART_SCR_DMAMODE_1 |
+			OMAP_UART_SCR_DMAMODE_CTL;
+		priv->scr |= OMAP_UART_SCR_TX_TRIG_GRANU1_MASK;
+	}
+
 	/*
 	 * NOTE: Setting OMAP_UART_SCR_RX_TRIG_GRANU1_MASK sets Enables the
 	 * granularity of 1 for TRIGGER RX level. Along with setting RX FIFO
@@ -283,13 +323,14 @@ static void omap_8250_set_termios(struct uart_port *port,
 	 * we receive an interrupt once TX FIFO (and shift) is empty as this is
 	 * what The irq routine currently expects to happen.
 	 */
-	priv->fcr = UART_FCR6_R_TRIGGER_16 | UART_FCR6_T_TRIGGER_24 |
-		UART_FCR_ENABLE_FIFO;
+	priv->fcr |= UART_FCR_ENABLE_FIFO;
+	priv->fcr |= TRIGGER_FCR_MASK(1) << OMAP_UART_FCR_TX_TRIG;
+	priv->fcr |= TRIGGER_FCR_MASK(8) << OMAP_UART_FCR_RX_TRIG;
 
 	serial_out(up, UART_FCR, priv->fcr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 
-	serial_out(up, UART_OMAP_SCR, priv->scr);
+	omap8250_update_scr(up, priv);
 
 	/* Reset UART_MCR_TCRTLR: this must be done with the EFR_ECB bit set */
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
@@ -333,13 +374,6 @@ static void omap_8250_set_termios(struct uart_port *port,
 	serial_out(up, UART_XON1, termios->c_cc[VSTART]);
 	serial_out(up, UART_XOFF1, termios->c_cc[VSTOP]);
 
-	/* Enable access to TCR/TLR */
-	serial_out(up, UART_EFR, UART_EFR_ECB);
-	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
-	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
-
-	serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
-
 	priv->efr = 0;
 	if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
 		/* Enable AUTORTS and AUTOCTS */
@@ -377,9 +411,8 @@ static void omap_8250_set_termios(struct uart_port *port,
 		else
 			up->mcr &= ~UART_MCR_XONANY;
 	}
-	serial_out(up, UART_MCR, up->mcr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
-	serial_out(up, UART_EFR, 0);
+	serial_out(up, UART_EFR, priv->efr);
 	serial_out(up, UART_LCR, up->lcr);
 
 	port->ops->set_mctrl(port, port->mctrl);
@@ -519,6 +552,9 @@ static int omap_8250_startup(struct uart_port *port)
 		priv->wer |= OMAP_UART_TX_WAKEUP_EN;
 	serial_out(up, UART_OMAP_WER, priv->wer);
 
+	if (up->dma)
+		serial8250_rx_dma(up, 0);
+
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
 	return 0;
@@ -580,6 +616,13 @@ static void omap_8250_unthrottle(struct uart_port *port)
 	pm_runtime_put_autosuspend(port->dev);
 }
 
+#ifdef CONFIG_SERIAL_8250_DMA
+static bool the_no_dma_filter_fn(struct dma_chan *chan, void *param)
+{
+	return false;
+}
+#endif
+
 static int omap8250_probe(struct platform_device *pdev)
 {
 	struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -667,7 +710,27 @@ static int omap8250_probe(struct platform_device *pdev)
 	pm_runtime_get_sync(&pdev->dev);
 
 	omap_serial_fill_features_erratas(&up, priv);
+#ifdef CONFIG_SERIAL_8250_DMA
+	if (pdev->dev.of_node) {
+		/*
+		 * Oh DMA support. If there are no DMA properties in the DT then
+		 * we will fall back to a generic DMA channel which does not
+		 * really work here. To ensure that we do not get a generic DMA
+		 * channel assigned, we have the the_no_dma_filter_fn() here.
+		 * To avoid "failed to request DMA" messages we check for DMA
+		 * properties in DT.
+		 */
+		ret = of_property_count_strings(pdev->dev.of_node, "dma-names");
+		if (ret == 2) {
+			up.dma = &priv->omap8250_dma;
+			priv->omap8250_dma.fn = the_no_dma_filter_fn;
+			priv->omap8250_dma.rx_size = 8;
+			priv->omap8250_dma.rxconf.src_maxburst = 8;
+			priv->omap8250_dma.txconf.dst_maxburst = 1;
 
+		}
+	}
+#endif
 	ret = serial8250_register_8250_port(&up);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "unable to register 8250 port\n");
@@ -838,7 +901,8 @@ static void omap8250_restore_context(struct omap8250_priv *priv)
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 	serial_out(up, UART_MCR, up->mcr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
-	serial_out(up, UART_OMAP_SCR, priv->scr);
+	omap8250_update_scr(up, priv);
+
 	serial_out(up, UART_EFR, priv->efr);
 	serial_out(up, UART_LCR, up->lcr);
 	if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
-- 
2.0.1

[RFC PATCH 7/7] tty: serial: 8250: omap: add dma support

[Sebastian Andrzej Siewior]

This patch adds the required pieces to OMAP uart for DMA support. The TX
burst size is set to 1 so we can send an arbitrary amout of bytes.

The RX burst is currently set to 8 which means we receive an interrupt
every eight bytes and have to reprogramm everything. Seven bytes in the
RX-fifo mean that no transfer will happen and the UART will send a
RX-timeout event at which point the FIFO will be manually purged.
I played around with the trigger value and burst and burst of one, trigger
eight isn't really better. We receive a DMA-complete event once we
received eight. Should we receive only 7 bytes then the DMA will fetch
them and wait for the last one and there will be RX-timeout from the
UART because the RX-FIFO is empty.
Since this makes most sense for high baudrates I think I will increment
the RX size to something larger.
This works fine on DRA7. AM33xx is a diffent story and I run into two
problems:
- TX, after ptogramming the TX transfer there has to be a byte written
  into the FIFO to trigger the transfer. After that, the transfer
  continues and reloads as expected. I had a workaround for this but
  wasn't working perfectly.
- RX, after the DMA engines fetches the data from the FIFO there is not
  only a dma-complete interrupt but also an UART interrupt which
  reports nothing (because the FIFO is already empty). I managed a few
  to receive wrong data if the UART interrupt was delayed and I am not
  yet sure if this is a real problem, a theoretical one or just a bug
  somewhere in the driver(s)

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_omap.c | 92 +++++++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 14 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 4d01f45..6cc8174 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -20,6 +20,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/console.h>
 #include <linux/pm_qos.h>
+#include <linux/edma.h>
 
 #include "8250.h"
 
@@ -31,10 +32,16 @@
 #define UART_ERRATA_i202_MDR1_ACCESS	(1 << 0)
 #define OMAP_UART_WER_HAS_TX_WAKEUP	(1 << 1)
 
+#define OMAP_UART_FCR_RX_TRIG		6
+#define OMAP_UART_FCR_TX_TRIG		4
+
 /* SCR register bitmasks */
 #define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK	(1 << 7)
 #define OMAP_UART_SCR_TX_TRIG_GRANU1_MASK	(1 << 6)
 #define OMAP_UART_SCR_TX_EMPTY			(1 << 3)
+#define OMAP_UART_SCR_DMAMODE_MASK		(3 << 1)
+#define OMAP_UART_SCR_DMAMODE_1			(1 << 1)
+#define OMAP_UART_SCR_DMAMODE_CTL		(1 << 0)
 
 /* MVR register bitmasks */
 #define OMAP_UART_MVR_SCHEME_SHIFT	30
@@ -45,6 +52,12 @@
 #define OMAP_UART_MVR_MAJ_SHIFT		8
 #define OMAP_UART_MVR_MIN_MASK		0x3f
 
+#define UART_TI752_TLR_TX	0
+#define UART_TI752_TLR_RX	4
+
+#define TRIGGER_TLR_MASK(x)	((x & 0x3c) >> 2)
+#define TRIGGER_FCR_MASK(x)	(x & 3)
+
 /* Enable XON/XOFF flow control on output */
 #define OMAP_UART_SW_TX		0x08
 /* Enable XON/XOFF flow control on input */
@@ -78,6 +91,7 @@ struct omap8250_priv {
 	u32 calc_latency;
 	struct pm_qos_request pm_qos_request;
 	struct work_struct qos_work;
+	struct uart_8250_dma omap8250_dma;
 };
 
 static u32 uart_read(struct uart_8250_port *up, u32 reg)
@@ -158,6 +172,20 @@ static void omap_8250_get_divisor(struct uart_port *port, unsigned int baud,
 	}
 }
 
+static void omap8250_update_scr(struct uart_8250_port *up,
+		struct omap8250_priv *priv)
+{
+	/*
+	 * As it turns out the DMA part in the UART strikes if the CONTROL bit
+	 * and the DMA-MODE is set at the same time. Therefore we first set the
+	 * control bit with DMA off and then we switch to the required DMA mode.
+	 */
+	if (priv->scr & OMAP_UART_SCR_DMAMODE_MASK)
+		serial_out(up, UART_OMAP_SCR,
+				priv->scr & ~OMAP_UART_SCR_DMAMODE_MASK);
+	serial_out(up, UART_OMAP_SCR, priv->scr);
+}
+
 /*
  * 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.
@@ -273,7 +301,19 @@ static void omap_8250_set_termios(struct uart_port *port,
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
 
-	priv->scr = OMAP_UART_SCR_RX_TRIG_GRANU1_MASK | OMAP_UART_SCR_TX_EMPTY;
+	serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
+
+	serial_out(up, UART_TI752_TLR,
+			TRIGGER_TLR_MASK(1) << UART_TI752_TLR_TX |
+			TRIGGER_TLR_MASK(8) << UART_TI752_TLR_RX);
+
+	priv->scr =  OMAP_UART_SCR_RX_TRIG_GRANU1_MASK | OMAP_UART_SCR_TX_EMPTY;
+	if (up->dma) {
+		priv->scr |= OMAP_UART_SCR_DMAMODE_1 |
+			OMAP_UART_SCR_DMAMODE_CTL;
+		priv->scr |= OMAP_UART_SCR_TX_TRIG_GRANU1_MASK;
+	}
+
 	/*
 	 * NOTE: Setting OMAP_UART_SCR_RX_TRIG_GRANU1_MASK sets Enables the
 	 * granularity of 1 for TRIGGER RX level. Along with setting RX FIFO
@@ -283,13 +323,14 @@ static void omap_8250_set_termios(struct uart_port *port,
 	 * we receive an interrupt once TX FIFO (and shift) is empty as this is
 	 * what The irq routine currently expects to happen.
 	 */
-	priv->fcr = UART_FCR6_R_TRIGGER_16 | UART_FCR6_T_TRIGGER_24 |
-		UART_FCR_ENABLE_FIFO;
+	priv->fcr |= UART_FCR_ENABLE_FIFO;
+	priv->fcr |= TRIGGER_FCR_MASK(1) << OMAP_UART_FCR_TX_TRIG;
+	priv->fcr |= TRIGGER_FCR_MASK(8) << OMAP_UART_FCR_RX_TRIG;
 
 	serial_out(up, UART_FCR, priv->fcr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 
-	serial_out(up, UART_OMAP_SCR, priv->scr);
+	omap8250_update_scr(up, priv);
 
 	/* Reset UART_MCR_TCRTLR: this must be done with the EFR_ECB bit set */
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
@@ -333,13 +374,6 @@ static void omap_8250_set_termios(struct uart_port *port,
 	serial_out(up, UART_XON1, termios->c_cc[VSTART]);
 	serial_out(up, UART_XOFF1, termios->c_cc[VSTOP]);
 
-	/* Enable access to TCR/TLR */
-	serial_out(up, UART_EFR, UART_EFR_ECB);
-	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
-	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
-
-	serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
-
 	priv->efr = 0;
 	if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
 		/* Enable AUTORTS and AUTOCTS */
@@ -377,9 +411,8 @@ static void omap_8250_set_termios(struct uart_port *port,
 		else
 			up->mcr &= ~UART_MCR_XONANY;
 	}
-	serial_out(up, UART_MCR, up->mcr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
-	serial_out(up, UART_EFR, 0);
+	serial_out(up, UART_EFR, priv->efr);
 	serial_out(up, UART_LCR, up->lcr);
 
 	port->ops->set_mctrl(port, port->mctrl);
@@ -519,6 +552,9 @@ static int omap_8250_startup(struct uart_port *port)
 		priv->wer |= OMAP_UART_TX_WAKEUP_EN;
 	serial_out(up, UART_OMAP_WER, priv->wer);
 
+	if (up->dma)
+		serial8250_rx_dma(up, 0);
+
 	pm_runtime_mark_last_busy(port->dev);
 	pm_runtime_put_autosuspend(port->dev);
 	return 0;
@@ -580,6 +616,13 @@ static void omap_8250_unthrottle(struct uart_port *port)
 	pm_runtime_put_autosuspend(port->dev);
 }
 
+#ifdef CONFIG_SERIAL_8250_DMA
+static bool the_no_dma_filter_fn(struct dma_chan *chan, void *param)
+{
+	return false;
+}
+#endif
+
 static int omap8250_probe(struct platform_device *pdev)
 {
 	struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -667,7 +710,27 @@ static int omap8250_probe(struct platform_device *pdev)
 	pm_runtime_get_sync(&pdev->dev);
 
 	omap_serial_fill_features_erratas(&up, priv);
+#ifdef CONFIG_SERIAL_8250_DMA
+	if (pdev->dev.of_node) {
+		/*
+		 * Oh DMA support. If there are no DMA properties in the DT then
+		 * we will fall back to a generic DMA channel which does not
+		 * really work here. To ensure that we do not get a generic DMA
+		 * channel assigned, we have the the_no_dma_filter_fn() here.
+		 * To avoid "failed to request DMA" messages we check for DMA
+		 * properties in DT.
+		 */
+		ret = of_property_count_strings(pdev->dev.of_node, "dma-names");
+		if (ret == 2) {
+			up.dma = &priv->omap8250_dma;
+			priv->omap8250_dma.fn = the_no_dma_filter_fn;
+			priv->omap8250_dma.rx_size = 8;
+			priv->omap8250_dma.rxconf.src_maxburst = 8;
+			priv->omap8250_dma.txconf.dst_maxburst = 1;
 
+		}
+	}
+#endif
 	ret = serial8250_register_8250_port(&up);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "unable to register 8250 port\n");
@@ -838,7 +901,8 @@ static void omap8250_restore_context(struct omap8250_priv *priv)
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
 	serial_out(up, UART_MCR, up->mcr);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
-	serial_out(up, UART_OMAP_SCR, priv->scr);
+	omap8250_update_scr(up, priv);
+
 	serial_out(up, UART_EFR, priv->efr);
 	serial_out(up, UART_LCR, up->lcr);
 	if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
-- 
2.0.1

Re: [PATCH 2/7] dmaengine: omap-dma: complete the transfer on terminate_all

[Sebastian Andrzej Siewior]

On 07/29/2014 08:58 PM, Sebastian Andrzej Siewior wrote:
> diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
> index b19f04f..81ede01 100644
> --- a/drivers/dma/omap-dma.c
> +++ b/drivers/dma/omap-dma.c
> @@ -970,6 +970,12 @@ static int omap_dma_terminate_all(struct omap_chan *c)
>  
>  	/* Prevent this channel being scheduled */
>  	spin_lock(&d->lock);
> +	if (!c->desc && !list_empty(&c->node)) {
> +		struct virt_dma_desc *vd = vchan_next_desc(&c->vc);
> +
> +		if (vd)
> +			dma_cookie_complete(&vd->tx);
> +	}

Why is start of the transfer deferred to the tasklet unless it is a
periodic transfer? Couldn't it be started right away?

>  	list_del_init(&c->node);
>  	spin_unlock(&d->lock);
>  

Sebastian

[PATCH 2/7] dmaengine: omap-dma: complete the transfer on terminate_all

[Sebastian Andrzej Siewior]

On 07/29/2014 08:58 PM, Sebastian Andrzej Siewior wrote:
> diff --git a/drivers/dma/omap-dma.c b/drivers/dma/omap-dma.c
> index b19f04f..81ede01 100644
> --- a/drivers/dma/omap-dma.c
> +++ b/drivers/dma/omap-dma.c
> @@ -970,6 +970,12 @@ static int omap_dma_terminate_all(struct omap_chan *c)
>  
>  	/* Prevent this channel being scheduled */
>  	spin_lock(&d->lock);
> +	if (!c->desc && !list_empty(&c->node)) {
> +		struct virt_dma_desc *vd = vchan_next_desc(&c->vc);
> +
> +		if (vd)
> +			dma_cookie_complete(&vd->tx);
> +	}

Why is start of the transfer deferred to the tasklet unless it is a
periodic transfer? Couldn't it be started right away?

>  	list_del_init(&c->node);
>  	spin_unlock(&d->lock);
>  

Sebastian

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Vinod Koul]

On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
> The rx path of the 8250_dma user in the RX-timeout case:
> - it starts the RX transfer
> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
> - step two is dmaengine_terminate_all() on this channel.
Okay after this whole channel needs to be reset, which means all the
descriptors are discared.
> - based on dmaengine_tx_status() it learns the number of transfered
>   bytes.
> - the rx interrupt occures,
why, channel is terminated, so existing txn needs to be aborted too

>   it does not start the RX-transfer because
>   according to dmaengine_tx_status() the status of the current transfer
>   is still DMA_IN_PROGRESS because the earlier dmaengine_terminate_all()
>   did not reset this.
there is no current transfer anymore

> - on rx-timeout it invokes dmaengine_pause() again. This time, it
>   segfaults because the channel has no descriptor yet.
> 
> To make the upper case work better, this patch adds dma_cookie_complete()
> to complete the cookie. Also it adds is an additional check for echan->edesc
> in case the channel has no descriptor assigned.
I think we are fixing the behvaiour rather than cause. terminate_all(()
needs to do a proper cleanup of the channel

And this looks a series, without cover letter sent to all. Which makes it a
bit hard to see what is getting done

-- 
~Vinod
> 
> Cc: Joel Fernandes <joelf@ti.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: dmaengine@vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/dma/edma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index d08c4de..ff05dd4 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -256,6 +256,7 @@ static int edma_terminate_all(struct edma_chan *echan)
>  	 * echan->edesc is NULL and exit.)
>  	 */
>  	if (echan->edesc) {
> +		dma_cookie_complete(&echan->edesc->vdesc.tx);
>  		echan->edesc = NULL;
>  		edma_stop(echan->ch_num);
>  	}
> @@ -282,7 +283,7 @@ static int edma_slave_config(struct edma_chan *echan,
>  static int edma_dma_pause(struct edma_chan *echan)
>  {
>  	/* Pause/Resume only allowed with cyclic mode */
> -	if (!echan->edesc->cyclic)
> +	if (!echan->edesc || !echan->edesc->cyclic)
>  		return -EINVAL;
>  
>  	edma_pause(echan->ch_num);
> -- 
> 2.0.1
> 

-- 

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Vinod Koul]

On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
> The rx path of the 8250_dma user in the RX-timeout case:
> - it starts the RX transfer
> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
> - step two is dmaengine_terminate_all() on this channel.
Okay after this whole channel needs to be reset, which means all the
descriptors are discared.
> - based on dmaengine_tx_status() it learns the number of transfered
>   bytes.
> - the rx interrupt occures,
why, channel is terminated, so existing txn needs to be aborted too

>   it does not start the RX-transfer because
>   according to dmaengine_tx_status() the status of the current transfer
>   is still DMA_IN_PROGRESS because the earlier dmaengine_terminate_all()
>   did not reset this.
there is no current transfer anymore

> - on rx-timeout it invokes dmaengine_pause() again. This time, it
>   segfaults because the channel has no descriptor yet.
> 
> To make the upper case work better, this patch adds dma_cookie_complete()
> to complete the cookie. Also it adds is an additional check for echan->edesc
> in case the channel has no descriptor assigned.
I think we are fixing the behvaiour rather than cause. terminate_all(()
needs to do a proper cleanup of the channel

And this looks a series, without cover letter sent to all. Which makes it a
bit hard to see what is getting done

-- 
~Vinod
> 
> Cc: Joel Fernandes <joelf@ti.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: dmaengine at vger.kernel.org
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/dma/edma.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index d08c4de..ff05dd4 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -256,6 +256,7 @@ static int edma_terminate_all(struct edma_chan *echan)
>  	 * echan->edesc is NULL and exit.)
>  	 */
>  	if (echan->edesc) {
> +		dma_cookie_complete(&echan->edesc->vdesc.tx);
>  		echan->edesc = NULL;
>  		edma_stop(echan->ch_num);
>  	}
> @@ -282,7 +283,7 @@ static int edma_slave_config(struct edma_chan *echan,
>  static int edma_dma_pause(struct edma_chan *echan)
>  {
>  	/* Pause/Resume only allowed with cyclic mode */
> -	if (!echan->edesc->cyclic)
> +	if (!echan->edesc || !echan->edesc->cyclic)
>  		return -EINVAL;
>  
>  	edma_pause(echan->ch_num);
> -- 
> 2.0.1
> 

-- 

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

On 07/31/2014 02:17 PM, Vinod Koul wrote:
> On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
>> The rx path of the 8250_dma user in the RX-timeout case:
>> - it starts the RX transfer
>> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
>> - step two is dmaengine_terminate_all() on this channel.
> Okay after this whole channel needs to be reset, which means all the
> descriptors are discared.
>> - based on dmaengine_tx_status() it learns the number of transfered
>>   bytes.
>> - the rx interrupt occures,
> why, channel is terminated, so existing txn needs to be aborted too

serial8250_rx_dma() is invoked on 8250' RX interrupt to move all the
data from the FIFO to memory. This is when the dma transfer is
programmed (on all platforms but omap because we need to program it
before the data is in the FIFO but this detail shouldn't matter).

On Omap (atleast) it happens that if there are not enough characters in
the FIFO over a given time the UART triggers a RX-timeout and no bytes
are moved by the DMA engine. This is when UART_IIR_RX_TIMEOUT is hit.
At that point it invokes the completion hanlder
(__dma_rx_do_complete()) to learn how much bytes were transfered and to
cancel the transfer (so the remaining bytes can be fetched via PIO).

>>   it does not start the RX-transfer because
>>   according to dmaengine_tx_status() the status of the current transfer
>>   is still DMA_IN_PROGRESS because the earlier dmaengine_terminate_all()
>>   did not reset this.
> there is no current transfer anymore

That is correct but since we never completed that cookie,
dmaengine_tx_status() returns DMA_IN_PROGRESS and the 8250_dma does no
book keeping of its own.

>> - on rx-timeout it invokes dmaengine_pause() again. This time, it
>>   segfaults because the channel has no descriptor yet.
>>
>> To make the upper case work better, this patch adds dma_cookie_complete()
>> to complete the cookie. Also it adds is an additional check for echan->edesc
>> in case the channel has no descriptor assigned.
> I think we are fixing the behvaiour rather than cause. terminate_all(()
> needs to do a proper cleanup of the channel

The channel is clean an can be reused. Since the cookie has never been
completed / incremented, serial8250_rx_dma() never enqueues another
transfer.

Are you suggesting that the 8250_dma driver should have its own book
keeping whether it started a transfer or not?

Either way, it looks wrong that dmaengine_tx_status() reports
DMA_IN_PROGRESS even that the transfer got aborted and the channel is
properly cleaned up.

> And this looks a series, without cover letter sent to all. Which makes it a
> bit hard to see what is getting done

What is getting done, is getting 8250_dma used on omap after the 8250
based driver is used for uart. The UART is mostly the same on am335x
(where edma is used) as on DRA7 (where omap-dma is used) that is why I
made the two patches for the two dma engines. The cover letter is at
[1]. I tried to cover the problem and all informations in the patch
description so you have all the required informations.

[0] drivers/tty/serial/8250/8250_dma.c
[1] https://lkml.org/lkml/2014/7/29/513

Sebastian

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

On 07/31/2014 02:17 PM, Vinod Koul wrote:
> On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
>> The rx path of the 8250_dma user in the RX-timeout case:
>> - it starts the RX transfer
>> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
>> - step two is dmaengine_terminate_all() on this channel.
> Okay after this whole channel needs to be reset, which means all the
> descriptors are discared.
>> - based on dmaengine_tx_status() it learns the number of transfered
>>   bytes.
>> - the rx interrupt occures,
> why, channel is terminated, so existing txn needs to be aborted too

serial8250_rx_dma() is invoked on 8250' RX interrupt to move all the
data from the FIFO to memory. This is when the dma transfer is
programmed (on all platforms but omap because we need to program it
before the data is in the FIFO but this detail shouldn't matter).

On Omap (atleast) it happens that if there are not enough characters in
the FIFO over a given time the UART triggers a RX-timeout and no bytes
are moved by the DMA engine. This is when UART_IIR_RX_TIMEOUT is hit.
At that point it invokes the completion hanlder
(__dma_rx_do_complete()) to learn how much bytes were transfered and to
cancel the transfer (so the remaining bytes can be fetched via PIO).

>>   it does not start the RX-transfer because
>>   according to dmaengine_tx_status() the status of the current transfer
>>   is still DMA_IN_PROGRESS because the earlier dmaengine_terminate_all()
>>   did not reset this.
> there is no current transfer anymore

That is correct but since we never completed that cookie,
dmaengine_tx_status() returns DMA_IN_PROGRESS and the 8250_dma does no
book keeping of its own.

>> - on rx-timeout it invokes dmaengine_pause() again. This time, it
>>   segfaults because the channel has no descriptor yet.
>>
>> To make the upper case work better, this patch adds dma_cookie_complete()
>> to complete the cookie. Also it adds is an additional check for echan->edesc
>> in case the channel has no descriptor assigned.
> I think we are fixing the behvaiour rather than cause. terminate_all(()
> needs to do a proper cleanup of the channel

The channel is clean an can be reused. Since the cookie has never been
completed / incremented, serial8250_rx_dma() never enqueues another
transfer.

Are you suggesting that the 8250_dma driver should have its own book
keeping whether it started a transfer or not?

Either way, it looks wrong that dmaengine_tx_status() reports
DMA_IN_PROGRESS even that the transfer got aborted and the channel is
properly cleaned up.

> And this looks a series, without cover letter sent to all. Which makes it a
> bit hard to see what is getting done

What is getting done, is getting 8250_dma used on omap after the 8250
based driver is used for uart. The UART is mostly the same on am335x
(where edma is used) as on DRA7 (where omap-dma is used) that is why I
made the two patches for the two dma engines. The cover letter is at
[1]. I tried to cover the problem and all informations in the patch
description so you have all the required informations.

[0] drivers/tty/serial/8250/8250_dma.c
[1] https://lkml.org/lkml/2014/7/29/513

Sebastian

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

* Vinod Koul | 2014-07-31 17:47:02 [+0530]:

>On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
>> The rx path of the 8250_dma user in the RX-timeout case:
>> - it starts the RX transfer
>> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
>> - step two is dmaengine_terminate_all() on this channel.
>Okay after this whole channel needs to be reset, which means all the
>descriptors are discared.

>> To make the upper case work better, this patch adds dma_cookie_complete()
>> to complete the cookie. Also it adds is an additional check for echan->edesc
>> in case the channel has no descriptor assigned.
>I think we are fixing the behvaiour rather than cause. terminate_all(()
>needs to do a proper cleanup of the channel

In case you are not ignoring me but $reason here is an example that does
not work (with both drivers);

    desc = dmaengine_prep_slave_single(rxchan, …);
    rx_cookie = dmaengine_submit(desc);
    dma_async_issue_pending(rxchan);

    ssleep(2);
    /* Now assume that the transfer did not start */
    st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
    /* st is now DMA_IN_PROGRESS as expected */

    dmaengine_terminate_all(rxchan);
    st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
    /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
     * it has been terminated / canceled
     */

Both dma driver clean up all / terminate all descriptors as required but
_none_ of them completes the cookie. As a result dma_cookie_status()
still thinks that the transfer is in progress.

Sebastian

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

* Vinod Koul | 2014-07-31 17:47:02 [+0530]:

>On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
>> The rx path of the 8250_dma user in the RX-timeout case:
>> - it starts the RX transfer
>> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
>> - step two is dmaengine_terminate_all() on this channel.
>Okay after this whole channel needs to be reset, which means all the
>descriptors are discared.

>> To make the upper case work better, this patch adds dma_cookie_complete()
>> to complete the cookie. Also it adds is an additional check for echan->edesc
>> in case the channel has no descriptor assigned.
>I think we are fixing the behvaiour rather than cause. terminate_all(()
>needs to do a proper cleanup of the channel

In case you are not ignoring me but $reason here is an example that does
not work (with both drivers);

    desc = dmaengine_prep_slave_single(rxchan, …);
    rx_cookie = dmaengine_submit(desc);
    dma_async_issue_pending(rxchan);

    ssleep(2);
    /* Now assume that the transfer did not start */
    st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
    /* st is now DMA_IN_PROGRESS as expected */

    dmaengine_terminate_all(rxchan);
    st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
    /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
     * it has been terminated / canceled
     */

Both dma driver clean up all / terminate all descriptors as required but
_none_ of them completes the cookie. As a result dma_cookie_status()
still thinks that the transfer is in progress.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

* Vinod Koul | 2014-07-31 17:47:02 [+0530]:

>On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
>> The rx path of the 8250_dma user in the RX-timeout case:
>> - it starts the RX transfer
>> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
>> - step two is dmaengine_terminate_all() on this channel.
>Okay after this whole channel needs to be reset, which means all the
>descriptors are discared.

>> To make the upper case work better, this patch adds dma_cookie_complete()
>> to complete the cookie. Also it adds is an additional check for echan->edesc
>> in case the channel has no descriptor assigned.
>I think we are fixing the behvaiour rather than cause. terminate_all(()
>needs to do a proper cleanup of the channel

In case you are not ignoring me but $reason here is an example that does
not work (with both drivers);

    desc = dmaengine_prep_slave_single(rxchan, ?);
    rx_cookie = dmaengine_submit(desc);
    dma_async_issue_pending(rxchan);

    ssleep(2);
    /* Now assume that the transfer did not start */
    st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
    /* st is now DMA_IN_PROGRESS as expected */

    dmaengine_terminate_all(rxchan);
    st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
    /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
     * it has been terminated / canceled
     */

Both dma driver clean up all / terminate all descriptors as required but
_none_ of them completes the cookie. As a result dma_cookie_status()
still thinks that the transfer is in progress.

Sebastian

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Vinod Koul]

On Fri, Aug 08, 2014 at 06:29:50PM +0200, Sebastian Andrzej Siewior wrote:
> * Vinod Koul | 2014-07-31 17:47:02 [+0530]:
> 
> >On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
> >> The rx path of the 8250_dma user in the RX-timeout case:
> >> - it starts the RX transfer
> >> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
> >> - step two is dmaengine_terminate_all() on this channel.
> >Okay after this whole channel needs to be reset, which means all the
> >descriptors are discared.
> 
> >> To make the upper case work better, this patch adds dma_cookie_complete()
> >> to complete the cookie. Also it adds is an additional check for echan->edesc
> >> in case the channel has no descriptor assigned.
> >I think we are fixing the behvaiour rather than cause. terminate_all(()
> >needs to do a proper cleanup of the channel
> 
> In case you are not ignoring me but $reason here is an example that does
> not work (with both drivers);
Sorry This seems to have slipped thru, wasn't intentional!
> 
>     desc = dmaengine_prep_slave_single(rxchan, …);
>     rx_cookie = dmaengine_submit(desc);
>     dma_async_issue_pending(rxchan);
> 
>     ssleep(2);
>     /* Now assume that the transfer did not start */
>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>     /* st is now DMA_IN_PROGRESS as expected */
> 
>     dmaengine_terminate_all(rxchan);
>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
and here is the culprit. You have terminated the channel. This means that
dmaengine driver is free to clean up all the allocated descriptors on the
channels and do whatever it decides to do with them.

You have already terminated the channel so what is the point in querying the
status of the cookie, which you shouldn't use anyway after invoking
terminate_all() as its result is not correct.

>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>      * it has been terminated / canceled
>      */
> 
> Both dma driver clean up all / terminate all descriptors as required but
> _none_ of them completes the cookie. As a result dma_cookie_status()
> still thinks that the transfer is in progress.

Btw how does it matter in the driver here if the transaction completed or
not after terminate_all() was invoked. What is the purpose of querying
status.

Thanks
-- 
~Vinod

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Vinod Koul]

On Fri, Aug 08, 2014 at 06:29:50PM +0200, Sebastian Andrzej Siewior wrote:
> * Vinod Koul | 2014-07-31 17:47:02 [+0530]:
> 
> >On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
> >> The rx path of the 8250_dma user in the RX-timeout case:
> >> - it starts the RX transfer
> >> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
> >> - step two is dmaengine_terminate_all() on this channel.
> >Okay after this whole channel needs to be reset, which means all the
> >descriptors are discared.
> 
> >> To make the upper case work better, this patch adds dma_cookie_complete()
> >> to complete the cookie. Also it adds is an additional check for echan->edesc
> >> in case the channel has no descriptor assigned.
> >I think we are fixing the behvaiour rather than cause. terminate_all(()
> >needs to do a proper cleanup of the channel
> 
> In case you are not ignoring me but $reason here is an example that does
> not work (with both drivers);
Sorry This seems to have slipped thru, wasn't intentional!
> 
>     desc = dmaengine_prep_slave_single(rxchan, …);
>     rx_cookie = dmaengine_submit(desc);
>     dma_async_issue_pending(rxchan);
> 
>     ssleep(2);
>     /* Now assume that the transfer did not start */
>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>     /* st is now DMA_IN_PROGRESS as expected */
> 
>     dmaengine_terminate_all(rxchan);
>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
and here is the culprit. You have terminated the channel. This means that
dmaengine driver is free to clean up all the allocated descriptors on the
channels and do whatever it decides to do with them.

You have already terminated the channel so what is the point in querying the
status of the cookie, which you shouldn't use anyway after invoking
terminate_all() as its result is not correct.

>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>      * it has been terminated / canceled
>      */
> 
> Both dma driver clean up all / terminate all descriptors as required but
> _none_ of them completes the cookie. As a result dma_cookie_status()
> still thinks that the transfer is in progress.

Btw how does it matter in the driver here if the transaction completed or
not after terminate_all() was invoked. What is the purpose of querying
status.

Thanks
-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Vinod Koul]

On Fri, Aug 08, 2014 at 06:29:50PM +0200, Sebastian Andrzej Siewior wrote:
> * Vinod Koul | 2014-07-31 17:47:02 [+0530]:
> 
> >On Tue, Jul 29, 2014 at 08:58:58PM +0200, Sebastian Andrzej Siewior wrote:
> >> The rx path of the 8250_dma user in the RX-timeout case:
> >> - it starts the RX transfer
> >> - if the rx-timeout interrupt occures, it dmaengine_pause() the transfer
> >> - step two is dmaengine_terminate_all() on this channel.
> >Okay after this whole channel needs to be reset, which means all the
> >descriptors are discared.
> 
> >> To make the upper case work better, this patch adds dma_cookie_complete()
> >> to complete the cookie. Also it adds is an additional check for echan->edesc
> >> in case the channel has no descriptor assigned.
> >I think we are fixing the behvaiour rather than cause. terminate_all(()
> >needs to do a proper cleanup of the channel
> 
> In case you are not ignoring me but $reason here is an example that does
> not work (with both drivers);
Sorry This seems to have slipped thru, wasn't intentional!
> 
>     desc = dmaengine_prep_slave_single(rxchan, ?);
>     rx_cookie = dmaengine_submit(desc);
>     dma_async_issue_pending(rxchan);
> 
>     ssleep(2);
>     /* Now assume that the transfer did not start */
>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>     /* st is now DMA_IN_PROGRESS as expected */
> 
>     dmaengine_terminate_all(rxchan);
>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
and here is the culprit. You have terminated the channel. This means that
dmaengine driver is free to clean up all the allocated descriptors on the
channels and do whatever it decides to do with them.

You have already terminated the channel so what is the point in querying the
status of the cookie, which you shouldn't use anyway after invoking
terminate_all() as its result is not correct.

>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>      * it has been terminated / canceled
>      */
> 
> Both dma driver clean up all / terminate all descriptors as required but
> _none_ of them completes the cookie. As a result dma_cookie_status()
> still thinks that the transfer is in progress.

Btw how does it matter in the driver here if the transaction completed or
not after terminate_all() was invoked. What is the purpose of querying
status.

Thanks
-- 
~Vinod

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

On 08/19/2014 05:12 PM, Vinod Koul wrote:
>>
>>     desc = dmaengine_prep_slave_single(rxchan, …);
>>     rx_cookie = dmaengine_submit(desc);
>>     dma_async_issue_pending(rxchan);
>>
>>     ssleep(2);
>>     /* Now assume that the transfer did not start */
>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>     /* st is now DMA_IN_PROGRESS as expected */
>>
>>     dmaengine_terminate_all(rxchan);
>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> and here is the culprit. You have terminated the channel. This means that
> dmaengine driver is free to clean up all the allocated descriptors on the
> channels and do whatever it decides to do with them.

descriptors, yes.

> You have already terminated the channel so what is the point in querying the
> status of the cookie, which you shouldn't use anyway after invoking
> terminate_all() as its result is not correct.

The point is to check (later, after terminate_all()) if there is an
outstanding DMA transfer or not _and_ how much data was really
transfered. Looking at edma it does not really support the latter if
the transfer is already completed. On the plus side the HW does not
support partly transfers :)

But where is it written that the life time of the cookie is limited?
Looking at the "cooking check" code there is no such thing. It is
simply compare of completed vs passed number but okay, this is an
implementation detail.
>From [0] it says under "4. Submit the transaction":

| This returns a cookie can be used to check the progress of DMA engine
| activity via other DMA engine calls not covered in this document.

no life time limit mentioned here. Which brings to the question: Why is
it okay to use the cookie after the transaction "terminated" normally
but not if it has been canceled?

And from [0] the API explanation  "4. … dma_async_is_tx_complete()":

|Note:
|    Not all DMA engine drivers can return reliable information for
|    a running DMA channel.  It is recommended that DMA engine users
|    pause or stop (via dmaengine_terminate_all) the channel before
|    using this API.

So the documentation says to use the cookie with
dma_async_is_tx_complete() and before doing so it is recommended that
the transfer should be paused or stopped. _Exactly_ what is done here.

>>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>>      * it has been terminated / canceled
>>      */
>>
>> Both dma driver clean up all / terminate all descriptors as required but
>> _none_ of them completes the cookie. As a result dma_cookie_status()
>> still thinks that the transfer is in progress.
> 
> Btw how does it matter in the driver here if the transaction completed or
> not after terminate_all() was invoked. What is the purpose of querying
> status.

In the RX interrupt (of the 8250 unit), the code checks if the transfer
has been already started or not via querying the status. So if it
returns DMA_COMPLETE then a new transfer will be started. If it returns
DMA_IN_PROGRESS then the code returns doing nothing because the DMA
engine should start moving data anytime now so the RX interrupt goes
away.

That means: If the transfer is canceled then it won't be started again.

Btw: the current (probably only) dma driver that is used by 8250-dma is
dw/core.c. That one does cookie complete on terminate:
 dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
 dma_cookie_complete().

That is why it works for them…

[0] Documentation/dmaengine.txt

> 
> Thanks
> 

Sebastian

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

On 08/19/2014 05:12 PM, Vinod Koul wrote:
>>
>>     desc = dmaengine_prep_slave_single(rxchan, …);
>>     rx_cookie = dmaengine_submit(desc);
>>     dma_async_issue_pending(rxchan);
>>
>>     ssleep(2);
>>     /* Now assume that the transfer did not start */
>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>     /* st is now DMA_IN_PROGRESS as expected */
>>
>>     dmaengine_terminate_all(rxchan);
>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> and here is the culprit. You have terminated the channel. This means that
> dmaengine driver is free to clean up all the allocated descriptors on the
> channels and do whatever it decides to do with them.

descriptors, yes.

> You have already terminated the channel so what is the point in querying the
> status of the cookie, which you shouldn't use anyway after invoking
> terminate_all() as its result is not correct.

The point is to check (later, after terminate_all()) if there is an
outstanding DMA transfer or not _and_ how much data was really
transfered. Looking at edma it does not really support the latter if
the transfer is already completed. On the plus side the HW does not
support partly transfers :)

But where is it written that the life time of the cookie is limited?
Looking at the "cooking check" code there is no such thing. It is
simply compare of completed vs passed number but okay, this is an
implementation detail.
From [0] it says under "4. Submit the transaction":

| This returns a cookie can be used to check the progress of DMA engine
| activity via other DMA engine calls not covered in this document.

no life time limit mentioned here. Which brings to the question: Why is
it okay to use the cookie after the transaction "terminated" normally
but not if it has been canceled?

And from [0] the API explanation  "4. … dma_async_is_tx_complete()":

|Note:
|    Not all DMA engine drivers can return reliable information for
|    a running DMA channel.  It is recommended that DMA engine users
|    pause or stop (via dmaengine_terminate_all) the channel before
|    using this API.

So the documentation says to use the cookie with
dma_async_is_tx_complete() and before doing so it is recommended that
the transfer should be paused or stopped. _Exactly_ what is done here.

>>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>>      * it has been terminated / canceled
>>      */
>>
>> Both dma driver clean up all / terminate all descriptors as required but
>> _none_ of them completes the cookie. As a result dma_cookie_status()
>> still thinks that the transfer is in progress.
> 
> Btw how does it matter in the driver here if the transaction completed or
> not after terminate_all() was invoked. What is the purpose of querying
> status.

In the RX interrupt (of the 8250 unit), the code checks if the transfer
has been already started or not via querying the status. So if it
returns DMA_COMPLETE then a new transfer will be started. If it returns
DMA_IN_PROGRESS then the code returns doing nothing because the DMA
engine should start moving data anytime now so the RX interrupt goes
away.

That means: If the transfer is canceled then it won't be started again.

Btw: the current (probably only) dma driver that is used by 8250-dma is
dw/core.c. That one does cookie complete on terminate:
 dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
 dma_cookie_complete().

That is why it works for them…

[0] Documentation/dmaengine.txt

> 
> Thanks
> 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

On 08/19/2014 05:12 PM, Vinod Koul wrote:
>>
>>     desc = dmaengine_prep_slave_single(rxchan, ?);
>>     rx_cookie = dmaengine_submit(desc);
>>     dma_async_issue_pending(rxchan);
>>
>>     ssleep(2);
>>     /* Now assume that the transfer did not start */
>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>     /* st is now DMA_IN_PROGRESS as expected */
>>
>>     dmaengine_terminate_all(rxchan);
>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> and here is the culprit. You have terminated the channel. This means that
> dmaengine driver is free to clean up all the allocated descriptors on the
> channels and do whatever it decides to do with them.

descriptors, yes.

> You have already terminated the channel so what is the point in querying the
> status of the cookie, which you shouldn't use anyway after invoking
> terminate_all() as its result is not correct.

The point is to check (later, after terminate_all()) if there is an
outstanding DMA transfer or not _and_ how much data was really
transfered. Looking at edma it does not really support the latter if
the transfer is already completed. On the plus side the HW does not
support partly transfers :)

But where is it written that the life time of the cookie is limited?
Looking at the "cooking check" code there is no such thing. It is
simply compare of completed vs passed number but okay, this is an
implementation detail.
>From [0] it says under "4. Submit the transaction":

| This returns a cookie can be used to check the progress of DMA engine
| activity via other DMA engine calls not covered in this document.

no life time limit mentioned here. Which brings to the question: Why is
it okay to use the cookie after the transaction "terminated" normally
but not if it has been canceled?

And from [0] the API explanation  "4. ? dma_async_is_tx_complete()":

|Note:
|    Not all DMA engine drivers can return reliable information for
|    a running DMA channel.  It is recommended that DMA engine users
|    pause or stop (via dmaengine_terminate_all) the channel before
|    using this API.

So the documentation says to use the cookie with
dma_async_is_tx_complete() and before doing so it is recommended that
the transfer should be paused or stopped. _Exactly_ what is done here.

>>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>>      * it has been terminated / canceled
>>      */
>>
>> Both dma driver clean up all / terminate all descriptors as required but
>> _none_ of them completes the cookie. As a result dma_cookie_status()
>> still thinks that the transfer is in progress.
> 
> Btw how does it matter in the driver here if the transaction completed or
> not after terminate_all() was invoked. What is the purpose of querying
> status.

In the RX interrupt (of the 8250 unit), the code checks if the transfer
has been already started or not via querying the status. So if it
returns DMA_COMPLETE then a new transfer will be started. If it returns
DMA_IN_PROGRESS then the code returns doing nothing because the DMA
engine should start moving data anytime now so the RX interrupt goes
away.

That means: If the transfer is canceled then it won't be started again.

Btw: the current (probably only) dma driver that is used by 8250-dma is
dw/core.c. That one does cookie complete on terminate:
 dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
 dma_cookie_complete().

That is why it works for them?

[0] Documentation/dmaengine.txt

> 
> Thanks
> 

Sebastian

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Vinod Koul]

On Thu, Aug 21, 2014 at 03:09:12PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/19/2014 05:12 PM, Vinod Koul wrote:
> >>
> >>     desc = dmaengine_prep_slave_single(rxchan, …);
> >>     rx_cookie = dmaengine_submit(desc);
> >>     dma_async_issue_pending(rxchan);
> >>
> >>     ssleep(2);
> >>     /* Now assume that the transfer did not start */
> >>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> >>     /* st is now DMA_IN_PROGRESS as expected */
> >>
> >>     dmaengine_terminate_all(rxchan);
> >>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> > and here is the culprit. You have terminated the channel. This means that
> > dmaengine driver is free to clean up all the allocated descriptors on the
> > channels and do whatever it decides to do with them.
> 
> descriptors, yes.
and by that logic when you query the driver would have freed up!

> > You have already terminated the channel so what is the point in querying the
> > status of the cookie, which you shouldn't use anyway after invoking
> > terminate_all() as its result is not correct.
> 
> The point is to check (later, after terminate_all()) if there is an
> outstanding DMA transfer or not _and_ how much data was really
> transfered. Looking at edma it does not really support the latter if
> the transfer is already completed. On the plus side the HW does not
> support partly transfers :)
well that can be achieved properly and differently!
Why don't we pause the channel, get the residue, status and then
terminate.

> But where is it written that the life time of the cookie is limited?
> Looking at the "cooking check" code there is no such thing. It is
> simply compare of completed vs passed number but okay, this is an
> implementation detail.
> From [0] it says under "4. Submit the transaction":
> 
> | This returns a cookie can be used to check the progress of DMA engine
> | activity via other DMA engine calls not covered in this document.
> 
> no life time limit mentioned here. Which brings to the question: Why is
> it okay to use the cookie after the transaction "terminated" normally
> but not if it has been canceled?
Due to the special nature of terminate. The point here is that you don't
terminate a transaction but channel operation

> And from [0] the API explanation  "4. … dma_async_is_tx_complete()":


> 
> |Note:
> |    Not all DMA engine drivers can return reliable information for
> |    a running DMA channel.  It is recommended that DMA engine users
> |    pause or stop (via dmaengine_terminate_all) the channel before
> |    using this API.
> 
> So the documentation says to use the cookie with
> dma_async_is_tx_complete() and before doing so it is recommended that
> the transfer should be paused or stopped. _Exactly_ what is done here.
> 
> >>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
> >>      * it has been terminated / canceled
> >>      */
> >>
> >> Both dma driver clean up all / terminate all descriptors as required but
> >> _none_ of them completes the cookie. As a result dma_cookie_status()
> >> still thinks that the transfer is in progress.
> > 
> > Btw how does it matter in the driver here if the transaction completed or
> > not after terminate_all() was invoked. What is the purpose of querying
> > status.
> 
> In the RX interrupt (of the 8250 unit), the code checks if the transfer
> has been already started or not via querying the status. So if it
> returns DMA_COMPLETE then a new transfer will be started. If it returns
> DMA_IN_PROGRESS then the code returns doing nothing because the DMA
> engine should start moving data anytime now so the RX interrupt goes
> away.
> 
> That means: If the transfer is canceled then it won't be started again.
> 
> Btw: the current (probably only) dma driver that is used by 8250-dma is
> dw/core.c. That one does cookie complete on terminate:
>  dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
>  dma_cookie_complete().
Yes but would above flow work for you :)

-- 
~Vinod

> 
> That is why it works for them…
> 
> [0] Documentation/dmaengine.txt
> 
> > 
> > Thanks
> > 
> 
> Sebastian

-- 

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Vinod Koul]

On Thu, Aug 21, 2014 at 03:09:12PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/19/2014 05:12 PM, Vinod Koul wrote:
> >>
> >>     desc = dmaengine_prep_slave_single(rxchan, ?);
> >>     rx_cookie = dmaengine_submit(desc);
> >>     dma_async_issue_pending(rxchan);
> >>
> >>     ssleep(2);
> >>     /* Now assume that the transfer did not start */
> >>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> >>     /* st is now DMA_IN_PROGRESS as expected */
> >>
> >>     dmaengine_terminate_all(rxchan);
> >>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
> > and here is the culprit. You have terminated the channel. This means that
> > dmaengine driver is free to clean up all the allocated descriptors on the
> > channels and do whatever it decides to do with them.
> 
> descriptors, yes.
and by that logic when you query the driver would have freed up!

> > You have already terminated the channel so what is the point in querying the
> > status of the cookie, which you shouldn't use anyway after invoking
> > terminate_all() as its result is not correct.
> 
> The point is to check (later, after terminate_all()) if there is an
> outstanding DMA transfer or not _and_ how much data was really
> transfered. Looking at edma it does not really support the latter if
> the transfer is already completed. On the plus side the HW does not
> support partly transfers :)
well that can be achieved properly and differently!
Why don't we pause the channel, get the residue, status and then
terminate.

> But where is it written that the life time of the cookie is limited?
> Looking at the "cooking check" code there is no such thing. It is
> simply compare of completed vs passed number but okay, this is an
> implementation detail.
> From [0] it says under "4. Submit the transaction":
> 
> | This returns a cookie can be used to check the progress of DMA engine
> | activity via other DMA engine calls not covered in this document.
> 
> no life time limit mentioned here. Which brings to the question: Why is
> it okay to use the cookie after the transaction "terminated" normally
> but not if it has been canceled?
Due to the special nature of terminate. The point here is that you don't
terminate a transaction but channel operation

> And from [0] the API explanation  "4. ? dma_async_is_tx_complete()":


> 
> |Note:
> |    Not all DMA engine drivers can return reliable information for
> |    a running DMA channel.  It is recommended that DMA engine users
> |    pause or stop (via dmaengine_terminate_all) the channel before
> |    using this API.
> 
> So the documentation says to use the cookie with
> dma_async_is_tx_complete() and before doing so it is recommended that
> the transfer should be paused or stopped. _Exactly_ what is done here.
> 
> >>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
> >>      * it has been terminated / canceled
> >>      */
> >>
> >> Both dma driver clean up all / terminate all descriptors as required but
> >> _none_ of them completes the cookie. As a result dma_cookie_status()
> >> still thinks that the transfer is in progress.
> > 
> > Btw how does it matter in the driver here if the transaction completed or
> > not after terminate_all() was invoked. What is the purpose of querying
> > status.
> 
> In the RX interrupt (of the 8250 unit), the code checks if the transfer
> has been already started or not via querying the status. So if it
> returns DMA_COMPLETE then a new transfer will be started. If it returns
> DMA_IN_PROGRESS then the code returns doing nothing because the DMA
> engine should start moving data anytime now so the RX interrupt goes
> away.
> 
> That means: If the transfer is canceled then it won't be started again.
> 
> Btw: the current (probably only) dma driver that is used by 8250-dma is
> dw/core.c. That one does cookie complete on terminate:
>  dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
>  dma_cookie_complete().
Yes but would above flow work for you :)

-- 
~Vinod

> 
> That is why it works for them?
> 
> [0] Documentation/dmaengine.txt
> 
> > 
> > Thanks
> > 
> 
> Sebastian

-- 

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

On 08/28/2014 09:06 AM, Vinod Koul wrote:
> On Thu, Aug 21, 2014 at 03:09:12PM +0200, Sebastian Andrzej Siewior wrote:
>> On 08/19/2014 05:12 PM, Vinod Koul wrote:
>>>>
>>>>     desc = dmaengine_prep_slave_single(rxchan, …);
>>>>     rx_cookie = dmaengine_submit(desc);
>>>>     dma_async_issue_pending(rxchan);
>>>>
>>>>     ssleep(2);
>>>>     /* Now assume that the transfer did not start */
>>>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>>>     /* st is now DMA_IN_PROGRESS as expected */
>>>>
>>>>     dmaengine_terminate_all(rxchan);
>>>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>> and here is the culprit. You have terminated the channel. This means that
>>> dmaengine driver is free to clean up all the allocated descriptors on the
>>> channels and do whatever it decides to do with them.
>>
>> descriptors, yes.
> and by that logic when you query the driver would have freed up!

I don't understand. What "would have freed up"?

>>> You have already terminated the channel so what is the point in querying the
>>> status of the cookie, which you shouldn't use anyway after invoking
>>> terminate_all() as its result is not correct.
>>
>> The point is to check (later, after terminate_all()) if there is an
>> outstanding DMA transfer or not _and_ how much data was really
>> transfered. Looking at edma it does not really support the latter if
>> the transfer is already completed. On the plus side the HW does not
>> support partly transfers :)
> well that can be achieved properly and differently!

I am all yours.

> Why don't we pause the channel, get the residue, status and then
> terminate.

This is done that way. For the protocol: pause is not supported by
driver (edma and omap-dma) unless the channel is used in "continuous"
mode. But this does not matter here.
The root problems remains: You get DMA_IN_PROGRESS returned as long as
the cookie has not been completed _even_ after the invocation of
terminate_all(). So that part that knows it terminates the thing for
the first time does everything correct.
The other part that only enqueues a transfer if there is not another
one pending does never do anything.

>> But where is it written that the life time of the cookie is limited?
>> Looking at the "cooking check" code there is no such thing. It is
>> simply compare of completed vs passed number but okay, this is an
>> implementation detail.
>> From [0] it says under "4. Submit the transaction":
>>
>> | This returns a cookie can be used to check the progress of DMA engine
>> | activity via other DMA engine calls not covered in this document.
>>
>> no life time limit mentioned here. Which brings to the question: Why is
>> it okay to use the cookie after the transaction "terminated" normally
>> but not if it has been canceled?
> Due to the special nature of terminate. The point here is that you don't
> terminate a transaction but channel operation

So the channel is terminated and the transaction is still in progress.
Is this what you are telling me? And why should a transaction remain in
progress after the channel has been terminated? I mean once you
terminate a bridge crossing point there are no cars going over that
bridge. It is closed. Their trip has been terminated as well.

The question that remains is which of those cars passed the bridge
before it was terminated. I understand that you want first to pause
that channel looking at the bytes and then terminate. Totally
understand. Otherwise the number of bytes transfered (reside) can no
longer be queried because the driver cleaned up. Fine.

But it should be reported that this transaction is no longer in
movement. It won't happen. Never. And this is what bugs me.

>> And from [0] the API explanation  "4. … dma_async_is_tx_complete()":
> 
> 
>>
>> |Note:
>> |    Not all DMA engine drivers can return reliable information for
>> |    a running DMA channel.  It is recommended that DMA engine users
>> |    pause or stop (via dmaengine_terminate_all) the channel before
>> |    using this API.
>>
>> So the documentation says to use the cookie with
>> dma_async_is_tx_complete() and before doing so it is recommended that
>> the transfer should be paused or stopped. _Exactly_ what is done here.
>>
>>>>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>>>>      * it has been terminated / canceled
>>>>      */
>>>>
>>>> Both dma driver clean up all / terminate all descriptors as required but
>>>> _none_ of them completes the cookie. As a result dma_cookie_status()
>>>> still thinks that the transfer is in progress.
>>>
>>> Btw how does it matter in the driver here if the transaction completed or
>>> not after terminate_all() was invoked. What is the purpose of querying
>>> status.
>>
>> In the RX interrupt (of the 8250 unit), the code checks if the transfer
>> has been already started or not via querying the status. So if it
>> returns DMA_COMPLETE then a new transfer will be started. If it returns
>> DMA_IN_PROGRESS then the code returns doing nothing because the DMA
>> engine should start moving data anytime now so the RX interrupt goes
>> away.
>>
>> That means: If the transfer is canceled then it won't be started again.
>>
>> Btw: the current (probably only) dma driver that is used by 8250-dma is
>> dw/core.c. That one does cookie complete on terminate:
>>  dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
>>  dma_cookie_complete().
> Yes but would above flow work for you :)

No, it wont. I ask you to accept the two patches since now the behavior
of dma_async_is_tx_complete() is different depending on whether a
transaction completed on its own or got terminated. And reporting
in_progress does not reflect the reality. I can even add a patch on top
updating the documentation where it state that you must not trust the
dma_tx_state struct once the request is completed (or aborted).

Or we can trade :) I send you a patch which removes the "cookie
complete" part from the dwc-dma driver with a CC stable tag.
And you explain it to the intel folks that merged that part of the 8250
dma code that rely on the fact (that a aborted transaction reports
complete) that what they do is wrong.

but I think it will end up with this: I will change the 8250-dma code to
keep track whether or not it enqueued a DMA transfer and rely on this
information instead of dma_async_is_tx_complete(). I will update the
documentation stating that dma_async_is_tx_complete() may report bogus
information on completed and terminated transaction.

Sebastian

Re: [PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

On 08/28/2014 09:06 AM, Vinod Koul wrote:
> On Thu, Aug 21, 2014 at 03:09:12PM +0200, Sebastian Andrzej Siewior wrote:
>> On 08/19/2014 05:12 PM, Vinod Koul wrote:
>>>>
>>>>     desc = dmaengine_prep_slave_single(rxchan, …);
>>>>     rx_cookie = dmaengine_submit(desc);
>>>>     dma_async_issue_pending(rxchan);
>>>>
>>>>     ssleep(2);
>>>>     /* Now assume that the transfer did not start */
>>>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>>>     /* st is now DMA_IN_PROGRESS as expected */
>>>>
>>>>     dmaengine_terminate_all(rxchan);
>>>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>> and here is the culprit. You have terminated the channel. This means that
>>> dmaengine driver is free to clean up all the allocated descriptors on the
>>> channels and do whatever it decides to do with them.
>>
>> descriptors, yes.
> and by that logic when you query the driver would have freed up!

I don't understand. What "would have freed up"?

>>> You have already terminated the channel so what is the point in querying the
>>> status of the cookie, which you shouldn't use anyway after invoking
>>> terminate_all() as its result is not correct.
>>
>> The point is to check (later, after terminate_all()) if there is an
>> outstanding DMA transfer or not _and_ how much data was really
>> transfered. Looking at edma it does not really support the latter if
>> the transfer is already completed. On the plus side the HW does not
>> support partly transfers :)
> well that can be achieved properly and differently!

I am all yours.

> Why don't we pause the channel, get the residue, status and then
> terminate.

This is done that way. For the protocol: pause is not supported by
driver (edma and omap-dma) unless the channel is used in "continuous"
mode. But this does not matter here.
The root problems remains: You get DMA_IN_PROGRESS returned as long as
the cookie has not been completed _even_ after the invocation of
terminate_all(). So that part that knows it terminates the thing for
the first time does everything correct.
The other part that only enqueues a transfer if there is not another
one pending does never do anything.

>> But where is it written that the life time of the cookie is limited?
>> Looking at the "cooking check" code there is no such thing. It is
>> simply compare of completed vs passed number but okay, this is an
>> implementation detail.
>> From [0] it says under "4. Submit the transaction":
>>
>> | This returns a cookie can be used to check the progress of DMA engine
>> | activity via other DMA engine calls not covered in this document.
>>
>> no life time limit mentioned here. Which brings to the question: Why is
>> it okay to use the cookie after the transaction "terminated" normally
>> but not if it has been canceled?
> Due to the special nature of terminate. The point here is that you don't
> terminate a transaction but channel operation

So the channel is terminated and the transaction is still in progress.
Is this what you are telling me? And why should a transaction remain in
progress after the channel has been terminated? I mean once you
terminate a bridge crossing point there are no cars going over that
bridge. It is closed. Their trip has been terminated as well.

The question that remains is which of those cars passed the bridge
before it was terminated. I understand that you want first to pause
that channel looking at the bytes and then terminate. Totally
understand. Otherwise the number of bytes transfered (reside) can no
longer be queried because the driver cleaned up. Fine.

But it should be reported that this transaction is no longer in
movement. It won't happen. Never. And this is what bugs me.

>> And from [0] the API explanation  "4. … dma_async_is_tx_complete()":
> 
> 
>>
>> |Note:
>> |    Not all DMA engine drivers can return reliable information for
>> |    a running DMA channel.  It is recommended that DMA engine users
>> |    pause or stop (via dmaengine_terminate_all) the channel before
>> |    using this API.
>>
>> So the documentation says to use the cookie with
>> dma_async_is_tx_complete() and before doing so it is recommended that
>> the transfer should be paused or stopped. _Exactly_ what is done here.
>>
>>>>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>>>>      * it has been terminated / canceled
>>>>      */
>>>>
>>>> Both dma driver clean up all / terminate all descriptors as required but
>>>> _none_ of them completes the cookie. As a result dma_cookie_status()
>>>> still thinks that the transfer is in progress.
>>>
>>> Btw how does it matter in the driver here if the transaction completed or
>>> not after terminate_all() was invoked. What is the purpose of querying
>>> status.
>>
>> In the RX interrupt (of the 8250 unit), the code checks if the transfer
>> has been already started or not via querying the status. So if it
>> returns DMA_COMPLETE then a new transfer will be started. If it returns
>> DMA_IN_PROGRESS then the code returns doing nothing because the DMA
>> engine should start moving data anytime now so the RX interrupt goes
>> away.
>>
>> That means: If the transfer is canceled then it won't be started again.
>>
>> Btw: the current (probably only) dma driver that is used by 8250-dma is
>> dw/core.c. That one does cookie complete on terminate:
>>  dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
>>  dma_cookie_complete().
> Yes but would above flow work for you :)

No, it wont. I ask you to accept the two patches since now the behavior
of dma_async_is_tx_complete() is different depending on whether a
transaction completed on its own or got terminated. And reporting
in_progress does not reflect the reality. I can even add a patch on top
updating the documentation where it state that you must not trust the
dma_tx_state struct once the request is completed (or aborted).

Or we can trade :) I send you a patch which removes the "cookie
complete" part from the dwc-dma driver with a CC stable tag.
And you explain it to the intel folks that merged that part of the 8250
dma code that rely on the fact (that a aborted transaction reports
complete) that what they do is wrong.

but I think it will end up with this: I will change the 8250-dma code to
keep track whether or not it enqueued a DMA transfer and rely on this
information instead of dma_async_is_tx_complete(). I will update the
documentation stating that dma_async_is_tx_complete() may report bogus
information on completed and terminated transaction.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/7] dmaengine: edma: fix two faults which happen with the 8250_dma user

[Sebastian Andrzej Siewior]

On 08/28/2014 09:06 AM, Vinod Koul wrote:
> On Thu, Aug 21, 2014 at 03:09:12PM +0200, Sebastian Andrzej Siewior wrote:
>> On 08/19/2014 05:12 PM, Vinod Koul wrote:
>>>>
>>>>     desc = dmaengine_prep_slave_single(rxchan, ?);
>>>>     rx_cookie = dmaengine_submit(desc);
>>>>     dma_async_issue_pending(rxchan);
>>>>
>>>>     ssleep(2);
>>>>     /* Now assume that the transfer did not start */
>>>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>>>     /* st is now DMA_IN_PROGRESS as expected */
>>>>
>>>>     dmaengine_terminate_all(rxchan);
>>>>     st = dmaengine_tx_status(rxchan, rx_cookie, NULL);
>>> and here is the culprit. You have terminated the channel. This means that
>>> dmaengine driver is free to clean up all the allocated descriptors on the
>>> channels and do whatever it decides to do with them.
>>
>> descriptors, yes.
> and by that logic when you query the driver would have freed up!

I don't understand. What "would have freed up"?

>>> You have already terminated the channel so what is the point in querying the
>>> status of the cookie, which you shouldn't use anyway after invoking
>>> terminate_all() as its result is not correct.
>>
>> The point is to check (later, after terminate_all()) if there is an
>> outstanding DMA transfer or not _and_ how much data was really
>> transfered. Looking at edma it does not really support the latter if
>> the transfer is already completed. On the plus side the HW does not
>> support partly transfers :)
> well that can be achieved properly and differently!

I am all yours.

> Why don't we pause the channel, get the residue, status and then
> terminate.

This is done that way. For the protocol: pause is not supported by
driver (edma and omap-dma) unless the channel is used in "continuous"
mode. But this does not matter here.
The root problems remains: You get DMA_IN_PROGRESS returned as long as
the cookie has not been completed _even_ after the invocation of
terminate_all(). So that part that knows it terminates the thing for
the first time does everything correct.
The other part that only enqueues a transfer if there is not another
one pending does never do anything.

>> But where is it written that the life time of the cookie is limited?
>> Looking at the "cooking check" code there is no such thing. It is
>> simply compare of completed vs passed number but okay, this is an
>> implementation detail.
>> From [0] it says under "4. Submit the transaction":
>>
>> | This returns a cookie can be used to check the progress of DMA engine
>> | activity via other DMA engine calls not covered in this document.
>>
>> no life time limit mentioned here. Which brings to the question: Why is
>> it okay to use the cookie after the transaction "terminated" normally
>> but not if it has been canceled?
> Due to the special nature of terminate. The point here is that you don't
> terminate a transaction but channel operation

So the channel is terminated and the transaction is still in progress.
Is this what you are telling me? And why should a transaction remain in
progress after the channel has been terminated? I mean once you
terminate a bridge crossing point there are no cars going over that
bridge. It is closed. Their trip has been terminated as well.

The question that remains is which of those cars passed the bridge
before it was terminated. I understand that you want first to pause
that channel looking at the bytes and then terminate. Totally
understand. Otherwise the number of bytes transfered (reside) can no
longer be queried because the driver cleaned up. Fine.

But it should be reported that this transaction is no longer in
movement. It won't happen. Never. And this is what bugs me.

>> And from [0] the API explanation  "4. ? dma_async_is_tx_complete()":
> 
> 
>>
>> |Note:
>> |    Not all DMA engine drivers can return reliable information for
>> |    a running DMA channel.  It is recommended that DMA engine users
>> |    pause or stop (via dmaengine_terminate_all) the channel before
>> |    using this API.
>>
>> So the documentation says to use the cookie with
>> dma_async_is_tx_complete() and before doing so it is recommended that
>> the transfer should be paused or stopped. _Exactly_ what is done here.
>>
>>>>     /* st is still DMA_IN_PROGRESS but _I_ expect DMA_COMPLETE because
>>>>      * it has been terminated / canceled
>>>>      */
>>>>
>>>> Both dma driver clean up all / terminate all descriptors as required but
>>>> _none_ of them completes the cookie. As a result dma_cookie_status()
>>>> still thinks that the transfer is in progress.
>>>
>>> Btw how does it matter in the driver here if the transaction completed or
>>> not after terminate_all() was invoked. What is the purpose of querying
>>> status.
>>
>> In the RX interrupt (of the 8250 unit), the code checks if the transfer
>> has been already started or not via querying the status. So if it
>> returns DMA_COMPLETE then a new transfer will be started. If it returns
>> DMA_IN_PROGRESS then the code returns doing nothing because the DMA
>> engine should start moving data anytime now so the RX interrupt goes
>> away.
>>
>> That means: If the transfer is canceled then it won't be started again.
>>
>> Btw: the current (probably only) dma driver that is used by 8250-dma is
>> dw/core.c. That one does cookie complete on terminate:
>>  dwc_control(DMA_TERMINATE_ALL) -> dwc_descriptor_complete() ->
>>  dma_cookie_complete().
> Yes but would above flow work for you :)

No, it wont. I ask you to accept the two patches since now the behavior
of dma_async_is_tx_complete() is different depending on whether a
transaction completed on its own or got terminated. And reporting
in_progress does not reflect the reality. I can even add a patch on top
updating the documentation where it state that you must not trust the
dma_tx_state struct once the request is completed (or aborted).

Or we can trade :) I send you a patch which removes the "cookie
complete" part from the dwc-dma driver with a CC stable tag.
And you explain it to the intel folks that merged that part of the 8250
dma code that rely on the fact (that a aborted transaction reports
complete) that what they do is wrong.

but I think it will end up with this: I will change the 8250-dma code to
keep track whether or not it enqueued a DMA transfer and rely on this
information instead of dma_async_is_tx_complete(). I will update the
documentation stating that dma_async_is_tx_complete() may report bogus
information on completed and terminated transaction.

Sebastian