[PATCH 0/4] serial: imx: some irq and handshaking fixes

[PATCH 0/4] serial: imx: some irq and handshaking fixes

[Uwe Kleine-König]

Hello,

this series doesn't solve all my problems but at least some. Now for my
remaining problem my machine at least doesn't reset (by watchdog) but
disables the serial irq instead which at least allows to debug further.
(Still I'm a bit out of ideas.)

The last patch doesn't fix a real problem I have, just noticed that
while working on the handshaking stuff.

It would be great if these patched made it into v4.6-rc1.

Best regards
Uwe

Uwe Kleine-K?nig (4):
  serial: imx: reorder functions to simplify next patch
  serial: imx: implement handshaking irq handling for DTE mode
  serial: imx: let irq handler return IRQ_NONE if no event was handled
  serial: imx: only count 0->1 transitions for RNG

 drivers/tty/serial/imx.c | 152 +++++++++++++++++++++++++++++------------------
 1 file changed, 95 insertions(+), 57 deletions(-)

-- 
2.7.0

[PATCH 1/4] serial: imx: reorder functions to simplify next patch

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/imx.c | 98 ++++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 49 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 231e7d5caf6c..ffa81e629a79 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -729,6 +729,55 @@ static void imx_dma_rxint(struct imx_port *sport)
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 }
 
+/*
+ * We have a modem side uart, so the meanings of RTS and CTS are inverted.
+ */
+static unsigned int imx_get_hwmctrl(struct imx_port *sport)
+{
+	unsigned int tmp = TIOCM_DSR;
+	unsigned usr1 = readl(sport->port.membase + USR1);
+
+	if (usr1 & USR1_RTSS)
+		tmp |= TIOCM_CTS;
+
+	/* in DCE mode DCDIN is always 0 */
+	if (!(usr1 & USR2_DCDIN))
+		tmp |= TIOCM_CAR;
+
+	/* in DCE mode RIIN is always 0 */
+	if (readl(sport->port.membase + USR2) & USR2_RIIN)
+		tmp |= TIOCM_RI;
+
+	return tmp;
+}
+
+/*
+ * Handle any change of modem status signal since we were last called.
+ */
+static void imx_mctrl_check(struct imx_port *sport)
+{
+	unsigned int status, changed;
+
+	status = imx_get_hwmctrl(sport);
+	changed = status ^ sport->old_status;
+
+	if (changed == 0)
+		return;
+
+	sport->old_status = status;
+
+	if (changed & TIOCM_RI)
+		sport->port.icount.rng++;
+	if (changed & TIOCM_DSR)
+		sport->port.icount.dsr++;
+	if (changed & TIOCM_CAR)
+		uart_handle_dcd_change(&sport->port, status & TIOCM_CAR);
+	if (changed & TIOCM_CTS)
+		uart_handle_cts_change(&sport->port, status & TIOCM_CTS);
+
+	wake_up_interruptible(&sport->port.state->port.delta_msr_wait);
+}
+
 static irqreturn_t imx_int(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
@@ -782,28 +831,6 @@ static unsigned int imx_tx_empty(struct uart_port *port)
 	return ret;
 }
 
-/*
- * We have a modem side uart, so the meanings of RTS and CTS are inverted.
- */
-static unsigned int imx_get_hwmctrl(struct imx_port *sport)
-{
-	unsigned int tmp = TIOCM_DSR;
-	unsigned usr1 = readl(sport->port.membase + USR1);
-
-	if (usr1 & USR1_RTSS)
-		tmp |= TIOCM_CTS;
-
-	/* in DCE mode DCDIN is always 0 */
-	if (!(usr1 & USR2_DCDIN))
-		tmp |= TIOCM_CAR;
-
-	/* in DCE mode RIIN is always 0 */
-	if (readl(sport->port.membase + USR2) & USR2_RIIN)
-		tmp |= TIOCM_RI;
-
-	return tmp;
-}
-
 static unsigned int imx_get_mctrl(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
@@ -861,33 +888,6 @@ static void imx_break_ctl(struct uart_port *port, int break_state)
 }
 
 /*
- * Handle any change of modem status signal since we were last called.
- */
-static void imx_mctrl_check(struct imx_port *sport)
-{
-	unsigned int status, changed;
-
-	status = imx_get_hwmctrl(sport);
-	changed = status ^ sport->old_status;
-
-	if (changed == 0)
-		return;
-
-	sport->old_status = status;
-
-	if (changed & TIOCM_RI)
-		sport->port.icount.rng++;
-	if (changed & TIOCM_DSR)
-		sport->port.icount.dsr++;
-	if (changed & TIOCM_CAR)
-		uart_handle_dcd_change(&sport->port, status & TIOCM_CAR);
-	if (changed & TIOCM_CTS)
-		uart_handle_cts_change(&sport->port, status & TIOCM_CTS);
-
-	wake_up_interruptible(&sport->port.state->port.delta_msr_wait);
-}
-
-/*
  * This is our per-port timeout handler, for checking the
  * modem status signals.
  */
-- 
2.7.0

[PATCH 2/4] serial: imx: implement handshaking irq handling for DTE mode

[Uwe Kleine-König]

Although this looks like a feature patch this is really a bug fix. In
DTE mode (i.e. with the devicetree property fsl,dte-mode) the interrupts
for RI and DCD are enabled because that's the reset default of these
bits (which have a different semantic in DCE mode) and the driver didn't
touch these bits before. The irq handler didn't handle these
subinterrupts and even fails to return IRQ_NONE in this case which
results in the machine being stuck when the RI or DCD pin changes level.
Still worse, when the respective pins are pinmuxed for a different
function, they include (at least on i.MX25) the SION bit, which means
the value at the pin is routed to the UART even then triggering the irq.

As of 4.5-rc7 there is only a single supported machine that operates an
UART in DTE mode and this UART doesn't have the respective pins, so no
in-tree-user should be affected.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
I already sent a patch to linux-arm-kernel ML that drops the SION bits.

 drivers/tty/serial/imx.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index ffa81e629a79..5586260059aa 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -114,6 +114,7 @@
 #define UCR3_RXDSEN	(1<<6)	/* Receive status interrupt enable */
 #define UCR3_AIRINTEN	(1<<5)	/* Async IR wake interrupt enable */
 #define UCR3_AWAKEN	(1<<4)	/* Async wake interrupt enable */
+#define UCR3_DTRDEN	(1<<3)	/* Data Terminal Ready Delta Enable. */
 #define IMX21_UCR3_RXDMUXSEL	(1<<2)	/* RXD Muxed Input Select */
 #define UCR3_INVT	(1<<1)	/* Inverted Infrared transmission */
 #define UCR3_BPEN	(1<<0)	/* Preset registers enable */
@@ -142,7 +143,7 @@
 #define USR1_FRAMERR	(1<<10) /* Frame error interrupt flag */
 #define USR1_RRDY	(1<<9)	 /* Receiver ready interrupt/dma flag */
 #define USR1_AGTIM	(1<<8)	 /* Ageing timer interrupt flag */
-#define USR1_TIMEOUT	(1<<7)	 /* Receive timeout interrupt status */
+#define USR1_DTRD	(1<<7)	 /* DTR Delta */
 #define USR1_RXDS	 (1<<6)	 /* Receiver idle interrupt flag */
 #define USR1_AIRINT	 (1<<5)	 /* Async IR wake interrupt flag */
 #define USR1_AWAKE	 (1<<4)	 /* Aysnc wake interrupt flag */
@@ -154,6 +155,7 @@
 #define USR2_RIIN	 (1<<9)	 /* Ring Indicator Input */
 #define USR2_IRINT	 (1<<8)	 /* Serial infrared interrupt flag */
 #define USR2_WAKE	 (1<<7)	 /* Wake */
+#define USR2_DCDDELT	 (1<<6)	 /* Data Carrier Detect Delta */
 #define USR2_DCDIN	 (1<<5)	 /* Data Carrier Detect Input */
 #define USR2_RTSF	 (1<<4)	 /* RTS edge interrupt flag */
 #define USR2_TXDC	 (1<<3)	 /* Transmitter complete */
@@ -800,6 +802,20 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 	     readl(sport->port.membase + UCR4) & UCR4_TCEN))
 		imx_txint(irq, dev_id);
 
+	if ((sts & USR1_DTRD) || (sts2 & (USR2_DCDDELT | USR2_RIDELT))) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&sport->port.lock, flags);
+		imx_mctrl_check(sport);
+		spin_unlock_irqrestore(&sport->port.lock, flags);
+		if (sts & USR1_DTRD)
+			writel(USR1_DTRD, sport->port.membase + USR1);
+
+		if (sts2 & (USR2_DCDDELT | USR2_RIDELT))
+			writel(sts2 & (USR2_DCDDELT | USR2_RIDELT),
+			       sport->port.membase + USR2);
+	}
+
 	if (sts & USR1_RTSD)
 		imx_rtsint(irq, dev_id);
 
@@ -1193,8 +1209,9 @@ static int imx_startup(struct uart_port *port)
 	/*
 	 * Finally, clear and enable interrupts
 	 */
-	writel(USR1_RTSD, sport->port.membase + USR1);
-	writel(USR2_ORE, sport->port.membase + USR2);
+	writel(USR1_RTSD | USR1_DTRD, sport->port.membase + USR1);
+	writel(USR2_ORE | USR2_DCDDELT | USR2_RIDELT,
+	       sport->port.membase + USR2);
 
 	if (sport->dma_is_inited && !sport->dma_is_enabled)
 		imx_enable_dma(sport);
@@ -1212,11 +1229,23 @@ static int imx_startup(struct uart_port *port)
 	temp |= (UCR2_RXEN | UCR2_TXEN);
 	if (!sport->have_rtscts)
 		temp |= UCR2_IRTS;
+	/* disable edge sensitive RTS-irq, we're using RTSD instead */
+	if (!is_imx1_uart(sport))
+		temp &= ~UCR2_RTSEN;
 	writel(temp, sport->port.membase + UCR2);
 
 	if (!is_imx1_uart(sport)) {
 		temp = readl(sport->port.membase + UCR3);
-		temp |= IMX21_UCR3_RXDMUXSEL | UCR3_ADNIMP;
+
+		/*
+		 * The effect of RI and DCD differs depending on the UFCR_DCEDTE
+		 * bit. In DCE mode they control the outputs, in DTE mode they
+		 * enable the respective irqs. For both modes it's correct to
+		 * set them (which matches the reset default).
+		 */
+		temp |= IMX21_UCR3_RXDMUXSEL | UCR3_ADNIMP |
+			UCR3_DTRDEN | UCR3_RI | UCR3_DCD;
+
 		writel(temp, sport->port.membase + UCR3);
 	}
 
-- 
2.7.0

[PATCH 3/4] serial: imx: let irq handler return IRQ_NONE if no event was handled

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/imx.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 5586260059aa..2e91598285dc 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -785,6 +785,7 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 	struct imx_port *sport = dev_id;
 	unsigned int sts;
 	unsigned int sts2;
+	irqreturn_t ret = IRQ_NONE;
 
 	sts = readl(sport->port.membase + USR1);
 	sts2 = readl(sport->port.membase + USR2);
@@ -799,8 +800,10 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 	if ((sts & USR1_TRDY &&
 	     readl(sport->port.membase + UCR1) & UCR1_TXMPTYEN) ||
 	    (sts2 & USR2_TXDC &&
-	     readl(sport->port.membase + UCR4) & UCR4_TCEN))
+	     readl(sport->port.membase + UCR4) & UCR4_TCEN)) {
 		imx_txint(irq, dev_id);
+		ret = IRQ_HANDLED;
+	}
 
 	if ((sts & USR1_DTRD) || (sts2 & (USR2_DCDDELT | USR2_RIDELT))) {
 		unsigned long flags;
@@ -814,20 +817,26 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 		if (sts2 & (USR2_DCDDELT | USR2_RIDELT))
 			writel(sts2 & (USR2_DCDDELT | USR2_RIDELT),
 			       sport->port.membase + USR2);
+		ret = IRQ_HANDLED;
 	}
 
-	if (sts & USR1_RTSD)
+	if (sts & USR1_RTSD) {
 		imx_rtsint(irq, dev_id);
+		ret = IRQ_HANDLED;
+	}
 
-	if (sts & USR1_AWAKE)
+	if (sts & USR1_AWAKE) {
 		writel(USR1_AWAKE, sport->port.membase + USR1);
+		ret = IRQ_HANDLED;
+	}
 
 	if (sts2 & USR2_ORE) {
 		sport->port.icount.overrun++;
 		writel(USR2_ORE, sport->port.membase + USR2);
+		ret = IRQ_HANDLED;
 	}
 
-	return IRQ_HANDLED;
+	return ret;
 }
 
 /*
-- 
2.7.0

[PATCH 4/4] serial: imx: only count 0->1 transitions for RNG

[Uwe Kleine-König]

According to tty_ioctl(4) (from man-pages 4.04) the rng member only
counts 0->1 transitions. For the other signals (DSR, CD, CTS) both edges
are supposed to be counted.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/serial/imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 2e91598285dc..d108fd30448a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -768,7 +768,7 @@ static void imx_mctrl_check(struct imx_port *sport)
 
 	sport->old_status = status;
 
-	if (changed & TIOCM_RI)
+	if (changed & TIOCM_RI && status & TIOCM_RI)
 		sport->port.icount.rng++;
 	if (changed & TIOCM_DSR)
 		sport->port.icount.dsr++;
-- 
2.7.0

[PATCH 0/4] serial: imx: some irq and handshaking fixes

[Uwe Kleine-König]

Hello Greg,

On Thu, Mar 10, 2016 at 11:26:05AM +0100, Uwe Kleine-K?nig wrote:
> this series doesn't solve all my problems but at least some. Now for my
> remaining problem my machine at least doesn't reset (by watchdog) but
> disables the serial irq instead which at least allows to debug further.
> (Still I'm a bit out of ideas.)
> 
> The last patch doesn't fix a real problem I have, just noticed that
> while working on the handshaking stuff.
> 
> It would be great if these patched made it into v4.6-rc1.

I have to rework this series. There is an unclear situation that results
in a stuck irq on i.MX25 because I'm unable to clear the DCD irq. Also I
missed a hunk in "let irq handler return IRQ_NONE if no event was
handled".

I will post a safer series later today. Please don't apply this one.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |