tty: serial: imx: disable ageing timer interrupt if dma in use

PATCH Superseded Troy Kisky 3 participants 5 messages Oct 7, 2017 – Oct 20, 2017

Subsystems (2)

Arm i.MX and Layerscape TTY layer and serial drivers

lore

Share & tools

lore

Troy Kisky

author

2017-10-07 01:46 UTC #0 lore raw

PATCH 1/1 tty: serial: imx: disable ageing timer interrupt if dma in use

Since commit 4dec2f119e86 ("imx-serial: RX DMA startup latency")

the interrupt routine no longer will start rx dma.

Signed-off-by Troy Kisky <[email protected]>

---
 drivers/tty/serial/imx.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

drivers/tty/serial/imx.c +2 ~~-32~~

index dfeff3951f93..15b0ecb4cf60 100644
@@ -732,29 +732,6 @@ static void imx_disable_rx_int(struct imx_port *sport)
 }
 
 static void clear_rx_errors(struct imx_port *sport);
-static int start_rx_dma(struct imx_port *sport);
-/*
- * If the RXFIFO is filled with some data, and then we
- * arise a DMA operation to receive them.
- */
-static void imx_dma_rxint(struct imx_port *sport)
-{
-	unsigned long temp;
-	unsigned long flags;
-
-	spin_lock_irqsave(&sport->port.lock, flags);
-
-	temp = readl(sport->port.membase + USR2);
-	if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
-
-		imx_disable_rx_int(sport);
-
-		/* tell the DMA to receive the data. */
-		start_rx_dma(sport);
-	}
-
-	spin_unlock_irqrestore(&sport->port.lock, flags);
-}
 
 /*
  * We have a modem side uart, so the meanings of RTS and CTS are inverted.

@@ -816,11 +793,8 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 	sts = readl(sport->port.membase + USR1);
 	sts2 = readl(sport->port.membase + USR2);
 
-	if (sts & (USR1_RRDY | USR1_AGTIM)) {
-		if (sport->dma_is_enabled)
-			imx_dma_rxint(sport);
-		else
-			imx_rxint(irq, dev_id);
+	if (!sport->dma_is_enabled && (sts & (USR1_RRDY | USR1_AGTIM))) {
+		imx_rxint(irq, dev_id);
 		ret = IRQ_HANDLED;
 	}

@@ -1207,10 +1181,6 @@ static void imx_enable_dma(struct imx_port *sport)
 	temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN;
 	writel(temp, sport->port.membase + UCR1);
 
-	temp = readl(sport->port.membase + UCR2);
-	temp |= UCR2_ATEN;
-	writel(temp, sport->port.membase + UCR2);
-
 	imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
 
 	sport->dma_is_enabled = 1;

Signature · 1 line

2.11.0

Han, Nandor (GE Healthcare)

2017-10-11 06:22 UTC #1 lore raw

PATCH 1/1 tty: serial: imx: disable ageing timer interrupt if dma in use

Quoted 13 lines

-----Original Message-----
From: linux-serial-owner at vger.kernel.org [mailto:linux-serial- owner at vger.kernel.org] On Behalf Of Troy Kisky
Sent: 07 October 2017 04:47
To: gregkh at linuxfoundation.org
Cc: fabio.estevam at nxp.com; l.stach at pengutronix.de; u.kleine- koenig at pengutronix.de; linux-serial at vger.kernel.org; linux-arm- kernel at lists.infradead.org; Troy Kisky <[email protected]>
Subject: EXT: [PATCH 1/1] tty: serial: imx: disable ageing timer interrupt if dma in use

Quoted 3 lines

Since commit 4dec2f119e86 ("imx-serial: RX DMA startup latency")
the interrupt routine no longer will start rx dma.

I think you have 2 commits here:
- one that disables ageing timer when DMA is in use
- one that cleans the leftovers after 4dec2f119e86.

Quoted 41 lines

Signed-off-by Troy Kisky <[email protected]>

---
 drivers/tty/serial/imx.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

drivers/tty/serial/imx.c index renamed from drivers/tty/serial/imx.c +0 -0

dfeff3951f93..15b0ecb4cf60 100644

drivers/tty/serial/imx.c +0 ~~-21~~

@@ -732,29 +732,6 @@ static void imx_disable_rx_int(struct imx_port
 *sport)  }
 
 static void clear_rx_errors(struct imx_port *sport); -static int
 start_rx_dma(struct imx_port *sport);
-/*
- * If the RXFIFO is filled with some data, and then we
- * arise a DMA operation to receive them.
- */
-static void imx_dma_rxint(struct imx_port *sport) -{
-	unsigned long temp;
-	unsigned long flags;
-
-	spin_lock_irqsave(&sport->port.lock, flags);
-
-	temp = readl(sport->port.membase + USR2);
-	if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
-
-		imx_disable_rx_int(sport);
-
-		/* tell the DMA to receive the data. */
-		start_rx_dma(sport);
-	}
-
-	spin_unlock_irqrestore(&sport->port.lock, flags);
-}
 
 /*
  * We have a modem side uart, so the meanings of RTS and CTS are inverted.

@@ -816,11 +793,8 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 	sts = readl(sport->port.membase + USR1);

drivers/tty/serial/imx.c static irqreturn_t imx_int(int irq, void *dev_id) · line 798

Quoted 10 lines

drivers/tty/serial/imx.c +2 -5

 	sts2 = readl(sport->port.membase + USR2);
 
-	if (sts & (USR1_RRDY | USR1_AGTIM)) {
-		if (sport->dma_is_enabled)
-			imx_dma_rxint(sport);
-		else
-			imx_rxint(irq, dev_id);
+	if (!sport->dma_is_enabled && (sts & (USR1_RRDY |
 USR1_AGTIM))) {
+		imx_rxint(irq, dev_id);

I don't think we need the check !sport->dma_is_enabled since RRDY and AGTIM interrupts should be disable when DMA is enabled.

drivers/tty/serial/imx.c static void imx_enable_dma(struct imx_port · line 1214

Quoted 12 lines

drivers/tty/serial/imx.c +0 -4

 		ret = IRQ_HANDLED;
 	}

@@ -1207,10 +1181,6 @@ static void imx_enable_dma(struct imx_port
 *sport)
 	temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN;
 	writel(temp, sport->port.membase + UCR1);
 
-	temp = readl(sport->port.membase + UCR2);
-	temp |= UCR2_ATEN;
-	writel(temp, sport->port.membase + UCR2);
-

I think this should be moved to imx_startup. At least RRDY is configured there.

<snip>

Regards,
Nandor

Troy Kisky

author

2017-10-11 16:09 UTC #2 lore raw

PATCH 1/1 tty: serial: imx: disable ageing timer interrupt if dma in use

Quoted 13 lines · Nandor (GE Healthcare)

Quoted 13 lines
-----Original Message-----
From: linux-serial-owner at vger.kernel.org [mailto:linux-serial- owner at vger.kernel.org] On Behalf Of Troy Kisky
Sent: 07 October 2017 04:47
To: gregkh at linuxfoundation.org
Cc: fabio.estevam at nxp.com; l.stach at pengutronix.de; u.kleine- koenig at pengutronix.de; linux-serial at vger.kernel.org; linux-arm- kernel at lists.infradead.org; Troy Kisky <[email protected]>
Subject: EXT: [PATCH 1/1] tty: serial: imx: disable ageing timer interrupt if dma in use

Quoted 8 lines

Quoted 3 lines
Since commit 4dec2f119e86 ("imx-serial: RX DMA startup latency")
the interrupt routine no longer will start rx dma.
I think you have 2 commits here:
- one that disables ageing timer when DMA is in use
- one that cleans the leftovers after 4dec2f119e86.

I could kill 3 lines, in the 1st patch, and the other 27 in the next. But the 2 patches would not be independent. You should not apply the second without the 1st. I think the change is simple enough not to warrant a series.

Quoted 43 lines

Signed-off-by Troy Kisky <[email protected]>

---
 drivers/tty/serial/imx.c | 34 ++--------------------------------
 1 file changed, 2 insertions(+), 32 deletions(-)

drivers/tty/serial/imx.c index renamed from drivers/tty/serial/imx.c +0 -0

dfeff3951f93..15b0ecb4cf60 100644

drivers/tty/serial/imx.c +0 ~~-21~~

@@ -732,29 +732,6 @@ static void imx_disable_rx_int(struct imx_port
 *sport)  }
 
 static void clear_rx_errors(struct imx_port *sport); -static int
 start_rx_dma(struct imx_port *sport);
-/*
- * If the RXFIFO is filled with some data, and then we
- * arise a DMA operation to receive them.
- */
-static void imx_dma_rxint(struct imx_port *sport) -{
-	unsigned long temp;
-	unsigned long flags;
-
-	spin_lock_irqsave(&sport->port.lock, flags);
-
-	temp = readl(sport->port.membase + USR2);
-	if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
-
-		imx_disable_rx_int(sport);
-
-		/* tell the DMA to receive the data. */
-		start_rx_dma(sport);
-	}
-
-	spin_unlock_irqrestore(&sport->port.lock, flags);
-}
 
 /*
  * We have a modem side uart, so the meanings of RTS and CTS are inverted.

@@ -816,11 +793,8 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 	sts = readl(sport->port.membase + USR1);
 	sts2 = readl(sport->port.membase + USR2);

Quoted 10 lines

drivers/tty/serial/imx.c static irqreturn_t imx_int(int irq, void *dev_id) · line 797
Quoted 8 lines
drivers/tty/serial/imx.c +2 -5
-	if (sts & (USR1_RRDY | USR1_AGTIM)) {
-		if (sport->dma_is_enabled)
-			imx_dma_rxint(sport);
-		else
-			imx_rxint(irq, dev_id);
+	if (!sport->dma_is_enabled && (sts & (USR1_RRDY |
 USR1_AGTIM))) {
+		imx_rxint(irq, dev_id);
I don't think we need the check !sport->dma_is_enabled since RRDY and AGTIM interrupts should be disable when DMA is enabled.

If I AND the status with its corresponding interrupt enable. But that is just as complicated, and more different from the original.

Quoted 9 lines

drivers/tty/serial/imx.c +0 -0

 		ret = IRQ_HANDLED;
 	}

@@ -1207,10 +1181,6 @@ static void imx_enable_dma(struct imx_port
 *sport)
 	temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN;
 	writel(temp, sport->port.membase + UCR1);

Quoted 7 lines

drivers/tty/serial/imx.c static void imx_enable_dma(struct imx_port *sport) · line 1213
Quoted 4 lines
drivers/tty/serial/imx.c +0 -4
-	temp = readl(sport->port.membase + UCR2);
-	temp |= UCR2_ATEN;
-	writel(temp, sport->port.membase + UCR2);
-
I think this should be moved to imx_startup. At least RRDY is configured there.

Currently, non-dma mode does not use the ageing timer. You get an interrupt immediately when 1 character is in the fifo. So, enabling the ageing timer and changing fifo level, should really be a separate patch. But it would be a nice patch.

Han, Nandor (GE Healthcare)

2017-10-13 14:47 UTC #3 lore raw

PATCH 1/1 tty: serial: imx: disable ageing timer interrupt if dma in use

Quoted 14 lines

-----Original Message-----
From: Troy Kisky [mailto:troy.kisky at boundarydevices.com]
Sent: 11 October 2017 19:10
To: Han, Nandor (GE Healthcare) <[email protected]>;
gregkh at linuxfoundation.org
Cc: fabio.estevam at nxp.com; l.stach at pengutronix.de; u.kleine- koenig at pengutronix.de; linux-serial at vger.kernel.org; linux-arm- kernel at lists.infradead.org
Subject: EXT: Re: [PATCH 1/1] tty: serial: imx: disable ageing timer interrupt if dma in use
On 10/10/2017 11:22 PM, Han, Nandor (GE Healthcare) wrote:

<snip>

Quoted 6 lines

Quoted 6 lines
drivers/tty/serial/imx.c static irqreturn_t imx_int(int irq, void *dev_id) · line 797
Quoted 4 lines
(quoted diff) +1 -1
-			imx_rxint(irq, dev_id);
+	if (!sport->dma_is_enabled && (sts & (USR1_RRDY |
USR1_AGTIM))) {
drivers/tty/serial/imx.c +1 -0
+		imx_rxint(irq, dev_id);
I don't think we need the check !sport->dma_is_enabled since RRDY and

Quoted 6 lines

AGTIM interrupts should be disable when DMA is enabled.
If I AND the status with its corresponding interrupt enable. But that is just as complicated, and more different from the original.

Yes, I agree, but it would be the right thing to do. :)

Quoted 15 lines

Quoted 9 lines
Quoted 9 lines
drivers/tty/serial/imx.c +0 -0
 		ret = IRQ_HANDLED;
 	}
@@ -1207,10 +1181,6 @@ static void imx_enable_dma(struct imx_port
 *sport)
 	temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN;
 	writel(temp, sport->port.membase + UCR1);
 
Quoted 6 lines
drivers/tty/serial/imx.c static void imx_enable_dma(struct imx_port *sport) · line 1213
Quoted 4 lines
drivers/tty/serial/imx.c +0 -4
-	temp = readl(sport->port.membase + UCR2);
-	temp |= UCR2_ATEN;
-	writel(temp, sport->port.membase + UCR2);
-
I think this should be moved to imx_startup. At least RRDY is configured

Quoted 10 lines

there.
Quoted 1 line
Currently, non-dma mode does not use the ageing timer. You get an interrupt immediately when
1 character is in the fifo. So, enabling the ageing timer and changing fifo level, should really be a separate patch. But it would be a nice patch.

Ok but in this case, we are left with aging timer and rrdy check in the interrupt and imx-startup enables only rrdy. (which is not much different than what it was before, since aging timer didn't get a chance to be triggered) I agree that changing the fifo level to give a chance also to the aging timer will be the right thing to do here.

Overall, I think is ok, my main concern with not enabling the aging timer in startup is that people can be confused about this.

Reviewed-by Nandor Han <[email protected]>

gregkh at linuxfoundation.org

maintainer

2017-10-20 11:56 UTC #4 lore raw

PATCH 1/1 tty: serial: imx: disable ageing timer interrupt if dma in use

In reply to Han, Nandor (GE Healthcare)

Quoted 13 lines · Nandor (GE Healthcare)

Quoted 13 lines
-----Original Message-----
From: linux-serial-owner at vger.kernel.org [mailto:linux-serial- owner at vger.kernel.org] On Behalf Of Troy Kisky
Sent: 07 October 2017 04:47
To: gregkh at linuxfoundation.org
Cc: fabio.estevam at nxp.com; l.stach at pengutronix.de; u.kleine- koenig at pengutronix.de; linux-serial at vger.kernel.org; linux-arm- kernel at lists.infradead.org; Troy Kisky <[email protected]>
Subject: EXT: [PATCH 1/1] tty: serial: imx: disable ageing timer interrupt if dma in use

Quoted 7 lines

Quoted 3 lines
Since commit 4dec2f119e86 ("imx-serial: RX DMA startup latency")
the interrupt routine no longer will start rx dma.
I think you have 2 commits here:
- one that disables ageing timer when DMA is in use
- one that cleans the leftovers after 4dec2f119e86.

I agree, Troy, please make a patch series here.

thanks,

greg k-h