Skip to contentSkip to conversation

serial: imx: handshaking fixes and improvments

PATCH Superseded Uwe Kleine-König 5 participants 27 messages Mar 24, 2016 – Nov 21, 2016 1 message not in the archive

Subsystems (3)
Device tree bindings Arm i.MX and Layerscape TTY layer and serial drivers
lore
Share & tools
lore

Uwe Kleine-König

author
#0 lore raw

v2 cover serial: imx: handshaking fixes and improvments

Hello,

this is the reworked series to address the handshaking problems I saw.

Among others it made my i.MX25 stuck when the other side toggles DCD. The first 4 patches are fixes that I'd like to see in 4.6-rcX, the last two patches are merge window material.

Best regards
Uwe

Uwe Kleine-K?nig (6):
serial: imx: fix polarity of RI
serial: imx: let irq handler return IRQ_NONE if no event was handled
serial: imx: make sure unhandled irqs are disabled
serial: imx: only count 0->1 transitions for RNG
serial: imx: reorder functions to simplify next patch
serial: imx: implement DSR irq handling for DTE mode

 drivers/tty/serial/imx.c | 156 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 100 insertions(+), 56 deletions(-)
Signature · 1 line
2.7.0
author
#1 lore raw

PATCH v2 1/6 serial: imx: fix polarity of RI

When in DTE mode, the bit USR2_RIIN is active low. So invert the logic accordingly.

Fixes 90ebc4838666 ("serial: imx: repair and complete handshaking")
Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/tty/serial/imx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +3 -3
index 231e7d5caf6c..bfc4555c63d1 100644
@@ -797,9 +797,9 @@ static unsigned int imx_get_hwmctrl(struct imx_port *sport)
 	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;
+	if (sport->dte_mode)
+		if (!(readl(sport->port.membase + USR2) & USR2_RIIN))
+			tmp |= TIOCM_RI;
 
 	return tmp;
 }
Signature · 1 line
2.7.0
author
#2 lore raw

PATCH v2 2/6 serial: imx: let irq handler return IRQ_NONE if no event was handled

In reply to Uwe Kleine-König

This gives the irq core a chance to disable the serial interrupt in case an event isn't cleared in the handler.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/tty/serial/imx.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/tty/serial/imx.c +13 -4
index bfc4555c63d1..5ced61e1de6a 100644
@@ -734,6 +734,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);

@@ -743,26 +744,34 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 			imx_dma_rxint(sport);
 		else
 			imx_rxint(irq, dev_id);
+		ret = IRQ_HANDLED;
 	}
 
 	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_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;
 }
 
 /*
Signature · 1 line
2.7.0
author
#3 lore raw

PATCH v2 3/6 serial: imx: make sure unhandled irqs are disabled

In reply to Uwe Kleine-König

Make sure that events that are not handled in the irq function don't trigger an interrupt.

When the serial port is operated in DTE mode, the events for DCD and RI events are enabled after a system reset by default.

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/tty/serial/imx.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
drivers/tty/serial/imx.c +22 -1
index 5ced61e1de6a..fcd48fd78bf5 100644
@@ -1221,11 +1221,32 @@ static int imx_startup(struct uart_port *port)
 	temp |= (UCR2_RXEN | UCR2_TXEN);
 	if (!sport->have_rtscts)
 		temp |= UCR2_IRTS;
+	/*
+	 * make sure the edge sensitive RTS-irq is disabled,
+	 * 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. At least the DCD irq cannot be
+		 * cleared on i.MX25 at least, so it's not usable and must be
+		 * disabled. I don't have test hardware to check if RI has the
+		 * same problem but I consider this likely so it's disabled for
+		 * now, too.
+		 */
+		temp |= IMX21_UCR3_RXDMUXSEL | UCR3_ADNIMP |
+			UCR3_RI | UCR3_DCD;
+
+		if (sport->dte_mode)
+			temp &= ~(UCR3_RI | UCR3_DCD);
+
 		writel(temp, sport->port.membase + UCR3);
 	}
Signature · 1 line
2.7.0
author
#4 lore raw

PATCH v2 4/6 serial: imx: only count 0->1 transitions for RNG

In reply to 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 <[email protected]>
---
 drivers/tty/serial/imx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/serial/imx.c +1 -1
index fcd48fd78bf5..1bf7b86acf66 100644
@@ -884,7 +884,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++;
Signature · 1 line
2.7.0
author
#5 lore raw

PATCH v2 5/6 serial: imx: reorder functions to simplify next patch

In reply to Uwe Kleine-König
Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/tty/serial/imx.c | 98 ++++++++++++++++++++++++------------------------
 1 file changed, 49 insertions(+), 49 deletions(-)
drivers/tty/serial/imx.c +49 -49
index 1bf7b86acf66..c07e652f8f58 100644
@@ -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;
+
+	if (sport->dte_mode)
+		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 && status & 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;

@@ -791,28 +840,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;
-
-	if (sport->dte_mode)
-		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;

@@ -870,33 +897,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 && status & 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.
  */
Signature · 1 line
2.7.0
author
#6 lore raw

PATCH v2 6/6 serial: imx: implement DSR irq handling for DTE mode

In reply to Uwe Kleine-König

Enable reporting of DSR events (which is named DTR in the registers because Freescale uses the names as seem from a DCE).

Signed-off-by Uwe Kleine-K?nig <[email protected]>
---
 drivers/tty/serial/imx.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +17 -3
index c07e652f8f58..621e488cbb12 100644
@@ -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 */

@@ -804,6 +805,19 @@ static irqreturn_t imx_int(int irq, void *dev_id)
 		ret = IRQ_HANDLED;
 	}
 
+	if (sts & USR1_DTRD) {
+		unsigned long flags;
+
+		if (sts & USR1_DTRD)
+			writel(USR1_DTRD, sport->port.membase + USR1);
+
+		spin_lock_irqsave(&sport->port.lock, flags);
+		imx_mctrl_check(sport);
+		spin_unlock_irqrestore(&sport->port.lock, flags);
+
+		ret = IRQ_HANDLED;
+	}
+
 	if (sts & USR1_RTSD) {
 		imx_rtsint(irq, dev_id);
 		ret = IRQ_HANDLED;

@@ -1202,7 +1216,7 @@ static int imx_startup(struct uart_port *port)
 	/*
 	 * Finally, clear and enable interrupts
 	 */
-	writel(USR1_RTSD, sport->port.membase + USR1);
+	writel(USR1_RTSD | USR1_DTRD, sport->port.membase + USR1);
 	writel(USR2_ORE, sport->port.membase + USR2);
 
 	if (sport->dma_is_inited && !sport->dma_is_enabled)

@@ -1242,7 +1256,7 @@ static int imx_startup(struct uart_port *port)
 		 * now, too.
 		 */
 		temp |= IMX21_UCR3_RXDMUXSEL | UCR3_ADNIMP |
-			UCR3_RI | UCR3_DCD;
+			UCR3_DTRDEN | UCR3_RI | UCR3_DCD;
 
 		if (sport->dte_mode)
 			temp &= ~(UCR3_RI | UCR3_DCD);
Signature · 1 line
2.7.0

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#8 lore raw

serial: imx: regression triggered by newly introduced DSR irq handling

Hello Christoph,

Cc += Shawn, Fabio

Quoted 6 lines · Christoph Fritz

here on a imx6sx board newly introduced DSR-irq-handling breaks serial console and network support.

Reverting commit 27e16501052e5341934d3 "serial: imx: implement DSR irq handling for DTE mode" fixes the issue.

Quoted 6 lines

The underlying cause is the RMII network configuration where register bit SION in IOMUXC_SW_MUX_CTL_PAD_ENET1_TX_CLK is necessary. But this also falsely feeds signal UART1_DTR_B and triggers DSR irq...

Can we revert this commit or add a quirk for RMII-SION configs or make this optional by a device tree option?

Hmm, MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK in arch/arm/boot/dts/imx6sx-pinfunc.h doesn't have SION set.

I remember there were issues around eth and SION on i.MX6DL, but don't know the details. A quick net research found
https://community.nxp.com/thread/359531 which has: "Set the SION bit. Note that this is not required because the funtion setting controls the signal path, but it is good practice as it reminds the user that the clock needs to fed back into the Ethernet MAC.". Sounds like dangerous smattering and a wrong expectation about "users".

https://community.nxp.com/thread/376821 is from someone who has the same problem(?) on i.MX6SX, but no solution yet.

Does that mean MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK always needs the SION bit set? Even if not, I'd like to have that documented there, similar to da4fa6fa8016 ("ARM: imx25-pinfunc: document SION being important for MX25_PAD_SD1_CMD__SD1_CMD").

Once this is done (and still an issue) I'd suggest to standardize a dt property

	disable-dsr;

(and the same for the other handshaking lines) and support th{is,ese} in the imx uart driver. Reverting 27e16501052e IMO isn't a sensible option.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#9 lore raw

serial: imx: regression triggered by newly introduced DSR irq handling

Quoted 25 lines · Uwe Kleine-K?nig
Quoted 6 lines · Christoph Fritz

here on a imx6sx board newly introduced DSR-irq-handling breaks serial console and network support.

Reverting commit 27e16501052e5341934d3 "serial: imx: implement DSR irq handling for DTE mode" fixes the issue.

Quoted 6 lines

The underlying cause is the RMII network configuration where register bit SION in IOMUXC_SW_MUX_CTL_PAD_ENET1_TX_CLK is necessary. But this also falsely feeds signal UART1_DTR_B and triggers DSR irq...

Can we revert this commit or add a quirk for RMII-SION configs or make this optional by a device tree option?

Hmm, MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK in arch/arm/boot/dts/imx6sx-pinfunc.h doesn't have SION set.

I remember there were issues around eth and SION on i.MX6DL, but don't know the details. A quick net research found
https://community.nxp.com/thread/359531 which has: "Set the SION bit. Note that this is not required because the funtion setting controls the signal path, but it is good practice as it reminds the user that the clock needs to fed back into the Ethernet MAC.". Sounds like dangerous smattering and a wrong expectation about "users".

Quoted 5 lines

https://community.nxp.com/thread/376821 is from someone who has the same problem(?) on i.MX6SX, but no solution yet.

Does that mean MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK always needs the SION bit set?

SION is necessary for pinconfig MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1 if bit 17 in IOMUXC_GPR_GPR1 is set and bit 13 not.

So that ref_enetpll1 provides a clock not only for the external PHY but also for the internal controller.

Using SION is a quirk here, because the silicon doesn't feed the clock back to the internal controller automatically.

On the other hand, NXP could argue: You need to add a wire on your PCB between ENET1_REF_CLK1 and ENET1_TX_CLK -- but referring to the datasheet ENET1_TX_CLK isn't used in RMII config...

So if Fabio or Shawn agrees with my assumption above, I'll add something like 27e16501052e5341934d3 "serial: imx: implement DSR irq handling for DTE mode".

Okay?

reviewer
#10 lore raw

serial: imx: regression triggered by newly introduced DSR irq handling

Hi Christoph,

Quoted 13 lines · Christoph Fritz

SION is necessary for pinconfig MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1 if bit 17 in IOMUXC_GPR_GPR1 is set and bit 13 not.

So that ref_enetpll1 provides a clock not only for the external PHY but also for the internal controller.

Using SION is a quirk here, because the silicon doesn't feed the clock back to the internal controller automatically.

On the other hand, NXP could argue: You need to add a wire on your PCB between ENET1_REF_CLK1 and ENET1_TX_CLK -- but referring to the datasheet ENET1_TX_CLK isn't used in RMII config...

Quoted 5 lines

So if Fabio or Shawn agrees with my assumption above, I'll add something like 27e16501052e5341934d3 "serial: imx: implement DSR irq handling for DTE mode".

Okay?

Could you please post your suggestion as a patch or RFC so that we can understand what your proposal is?

Thanks

#11 lore raw

serial: imx: regression triggered by newly introduced DSR irq handling

Quoted 12 lines · Fabio Estevam
Quoted 9 lines · Christoph Fritz

SION is necessary for pinconfig MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1 if bit 17 in IOMUXC_GPR_GPR1 is set and bit 13 not.

So that ref_enetpll1 provides a clock not only for the external PHY but also for the internal controller.

Using SION is a quirk here, because the silicon doesn't feed the clock back to the internal controller automatically.

Quoted 7 lines
Quoted 7 lines

On the other hand, NXP could argue: You need to add a wire on your PCB between ENET1_REF_CLK1 and ENET1_TX_CLK -- but referring to the datasheet ENET1_TX_CLK isn't used in RMII config...

So if Fabio or Shawn agrees with my assumption above, I'll add something like 27e16501052e5341934d3 "serial: imx: implement DSR irq handling for DTE mode".

To correct myself, I'm referring to commit da4fa6fa8016dba54ae "ARM:
imx25-pinfunc: document SION being important for MX25_PAD_SD1_CMD__SD1_CMD".

Quoted 5 lines
Quoted 2 lines

Okay?

Could you please post your suggestion as a patch or RFC so that we can understand what your proposal is?

Sure, here is my proposal:

---
 arch/arm/boot/dts/imx6sx-pinfunc.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)
arch/arm/boot/dts/imx6sx-pinfunc.h +13 -0
index bb9c6b7..67a3dd7 100644
@@ -308,6 +308,19 @@
 #define MX6SX_PAD_ENET1_RX_CLK__VDEC_DEBUG_35                     0x008C 0x03D4 0x0000 0x8 0x0
 #define MX6SX_PAD_ENET1_RX_CLK__PCIE_CTRL_DEBUG_29                0x008C 0x03D4 0x0000 0x9 0x0
 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK                      0x0090 0x03D8 0x0000 0x0 0x0
+/*
+ * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is
+ * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a
+ * PHY in RMII mode. This configuration is true if:
+ *  - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set
+ *  - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset
+ * It seems to be a silicon bug that in this configuration ENET1_TX reference
+ * clock isn't provided automatically.  According to i.mx6sx reference manual
+ * (IOMUXC_GPR_GPR1 field descriptions: ENET1_CLK_SEL, Rev. 0 from 2/2015) it
+ * should be the case.
+ * This might have side effects for other hardware units that are connected to
+ * that pin and use the respective function as input (e.g. DSR irq handling).
+ */
 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1                    0x0090 0x03D8 0x0760 0x1 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__AUDMUX_AUD4_RXD                   0x0090 0x03D8 0x0644 0x2 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B                       0x0090 0x03D8 0x0000 0x3 0x0

--

author
#12 lore raw

serial: imx: regression triggered by newly introduced DSR irq handling

Hello Christoph,

Quoted 5 lines · Christoph Fritz
Quoted 2 lines · Fabio Estevam

Could you please post your suggestion as a patch or RFC so that we can understand what your proposal is?

Sure, here is my proposal:

I like it, just a few minor nits below.

Quoted 4 lines

+/*
+ * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is
+ * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a
+ * PHY in RMII mode. This configuration is true if:

Sounds a bit strage to me. Maybe s/true/necessary/? Or "valid"?

Quoted 4 lines

+ * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set
+ * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset
+ * It seems to be a silicon bug that in this configuration ENET1_TX reference
+ * clock isn't provided automatically. According to i.mx6sx reference manual

s/i.mx6sx/i.MX6SX/

Quoted 4 lines

+ * (IOMUXC_GPR_GPR1 field descriptions: ENET1_CLK_SEL, Rev. 0 from 2/2015) it
+ * should be the case.
+ * This might have side effects for other hardware units that are connected to
+ * that pin and use the respective function as input (e.g. DSR irq handling).

s/DSR irq/UART1's DTR/. Then it's more obvious to relate to MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B.

Quoted 4 lines

+ */

 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1                    0x0090 0x03D8 0x0760 0x1 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__AUDMUX_AUD4_RXD                   0x0090 0x03D8 0x0644 0x2 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B                       0x0090 0x03D8 0x0000 0x3 0x0

Thanks
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#13 lore raw

PATCH 1/1 ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1

Signed-off-by Christoph Fritz <[email protected]>
---
 arch/arm/boot/dts/imx6sx-pinfunc.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
arch/arm/boot/dts/imx6sx-pinfunc.h +14 -0
index bb9c6b7..42c4c80 100644
@@ -308,6 +308,20 @@
 #define MX6SX_PAD_ENET1_RX_CLK__VDEC_DEBUG_35                     0x008C 0x03D4 0x0000 0x8 0x0
 #define MX6SX_PAD_ENET1_RX_CLK__PCIE_CTRL_DEBUG_29                0x008C 0x03D4 0x0000 0x9 0x0
 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK                      0x0090 0x03D8 0x0000 0x0 0x0
+/*
+ * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is
+ * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a
+ * PHY in RMII mode. This configuration is valid if:
+ *  - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set
+ *  - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset
+ * It seems to be a silicon bug that in this configuration ENET1_TX reference
+ * clock isn't provided automatically.  According to i.MX6SX reference manual
+ * (IOMUXC_GPR_GPR1 field descriptions: ENET1_CLK_SEL, Rev. 0 from 2/2015) it
+ * should be the case.
+ * So this might have unwanted side effects for other hardware units that are
+ * also connected to that pin and using respective function as input (e.g.
+ * UART1's DTR handling on MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B).
+ */
 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1                    0x0090 0x03D8 0x0760 0x1 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__AUDMUX_AUD4_RXD                   0x0090 0x03D8 0x0644 0x2 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B                       0x0090 0x03D8 0x0000 0x3 0x0
Signature · 1 line
2.1.4
reviewer
#14 lore raw

PATCH 1/1 ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1

Hi Christoph,

Quoted 6 lines · Christoph Fritz

+/*
+ * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is
+ * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a
+ * PHY in RMII mode. This configuration is valid if:
+ * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set
+ * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset

So in your case the imx6sx_enet_clk_sel() does not do what you need:

static void __init imx6sx_enet_clk_sel(void)

{
    struct regmap *gpr;

    gpr = syscon_regmap_lookup_by_compatible("fsl,imx6sx-iomuxc-gpr");
    if (!IS_ERR(gpr)) {
        regmap_update_bits(gpr, IOMUXC_GPR1,
                   IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK, 0);
        regmap_update_bits(gpr, IOMUXC_GPR1,
                   IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK, 0);
    } else {
        pr_err("failed to find fsl,imx6sx-iomux-gpr regmap\n");
    }
}

It seems that it is not a good idea to have imx6sx_enet_clk_sel() in common code as the GPR1 setting can change from board to board.

I think we need the fec driver to be in charge of configuring the GPR1 register.

This is unrelated to your patch though :-)

#15 lore raw

PATCH 1/1 ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1

Quoted 29 lines · Fabio Estevam

Hi Christoph,

Quoted 6 lines · Christoph Fritz

+/*
+ * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is
+ * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a
+ * PHY in RMII mode. This configuration is valid if:
+ * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set
+ * - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset

So in your case the imx6sx_enet_clk_sel() does not do what you need:

static void __init imx6sx_enet_clk_sel(void)

{
    struct regmap *gpr;

    gpr = syscon_regmap_lookup_by_compatible("fsl,imx6sx-iomuxc-gpr");
    if (!IS_ERR(gpr)) {
        regmap_update_bits(gpr, IOMUXC_GPR1,
                   IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK, 0);
        regmap_update_bits(gpr, IOMUXC_GPR1,
                   IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK, 0);
    } else {
        pr_err("failed to find fsl,imx6sx-iomux-gpr regmap\n");
    }
}
Quoted 2 lines

It seems that it is not a good idea to have imx6sx_enet_clk_sel() in common code as the GPR1 setting can change from board to board.

Yes, currently I'm adapting/quirking the fec config there and in imx6sx_clocks_init() to set the Phy-Clock-Frequency to 50Mhz.

Quoted 1 line

I think we need the fec driver to be in charge of configuring the GPR1 register.

What about making the Phy-Clock-Frequency IMX6SX_CLK_ENET_REF adjustable?

Quoted 1 line

This is unrelated to your patch though :-)

Thanks
-- Christoph

#16 lore raw

PATCH 1/1 ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1

In reply to Christoph Fritz
Quoted 20 lines · Christoph Fritz
Signed-off-by Christoph Fritz <[email protected]>
---
 arch/arm/boot/dts/imx6sx-pinfunc.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
arch/arm/boot/dts/imx6sx-pinfunc.h +7 -0
index bb9c6b7..42c4c80 100644
@@ -308,6 +308,20 @@
 #define MX6SX_PAD_ENET1_RX_CLK__VDEC_DEBUG_35                     0x008C 0x03D4 0x0000 0x8 0x0
 #define MX6SX_PAD_ENET1_RX_CLK__PCIE_CTRL_DEBUG_29                0x008C 0x03D4 0x0000 0x9 0x0
 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK                      0x0090 0x03D8 0x0000 0x0 0x0
+/*
+ * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is
+ * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a
+ * PHY in RMII mode. This configuration is valid if:
+ *  - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set
+ *  - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset
+ * It seems to be a silicon bug that in this configuration ENET1_TX reference

arch/arm/boot/dts/imx6sx-pinfunc.h line 327

Quoted 10 lines
arch/arm/boot/dts/imx6sx-pinfunc.h +7 -0
+ * clock isn't provided automatically.  According to i.MX6SX reference manual
+ * (IOMUXC_GPR_GPR1 field descriptions: ENET1_CLK_SEL, Rev. 0 from 2/2015) it
+ * should be the case.
+ * So this might have unwanted side effects for other hardware units that are
+ * also connected to that pin and using respective function as input (e.g.
+ * UART1's DTR handling on MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B).
+ */
 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1                    0x0090 0x03D8 0x0760 0x1 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__AUDMUX_AUD4_RXD                   0x0090 0x03D8 0x0644 0x2 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B                       0x0090 0x03D8 0x0000 0x3 0x0

@Uwe and Fabio: Can I get your Ack on this?
@Shawn: Can you queue this patch for upstream?

Applied
#17 lore raw

PATCH 1/1 ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1

In reply to Christoph Fritz
Quoted 2 lines · Christoph Fritz
Signed-off-by Christoph Fritz <[email protected]>

Applied, thanks.

Quoted 37 lines
---
 arch/arm/boot/dts/imx6sx-pinfunc.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
arch/arm/boot/dts/imx6sx-pinfunc.h +14 -0
index bb9c6b7..42c4c80 100644
@@ -308,6 +308,20 @@
 #define MX6SX_PAD_ENET1_RX_CLK__VDEC_DEBUG_35                     0x008C 0x03D4 0x0000 0x8 0x0
 #define MX6SX_PAD_ENET1_RX_CLK__PCIE_CTRL_DEBUG_29                0x008C 0x03D4 0x0000 0x9 0x0
 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_TX_CLK                      0x0090 0x03D8 0x0000 0x0 0x0
+/*
+ * SION bit is necessary for ENET1_REF_CLK1 (ENET2_REF_CLK2 untested) if it is
+ * used as clock output of IMX6SX_CLK_ENET_REF (ENET1_TX_CLK) to e.g. supply a
+ * PHY in RMII mode. This configuration is valid if:
+ *  - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_PAD_DIR_MASK is set
+ *  - bit 1 in field IMX6SX_GPR1_FEC_CLOCK_MUX_SEL_MASK unset
+ * It seems to be a silicon bug that in this configuration ENET1_TX reference
+ * clock isn't provided automatically.  According to i.MX6SX reference manual
+ * (IOMUXC_GPR_GPR1 field descriptions: ENET1_CLK_SEL, Rev. 0 from 2/2015) it
+ * should be the case.
+ * So this might have unwanted side effects for other hardware units that are
+ * also connected to that pin and using respective function as input (e.g.
+ * UART1's DTR handling on MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B).
+ */
 #define MX6SX_PAD_ENET1_TX_CLK__ENET1_REF_CLK1                    0x0090 0x03D8 0x0760 0x1 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__AUDMUX_AUD4_RXD                   0x0090 0x03D8 0x0644 0x2 0x1
 #define MX6SX_PAD_ENET1_TX_CLK__UART1_DTR_B                       0x0090 0x03D8 0x0000 0x3 0x0
Signature · 7 lines
2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#18 lore raw

serial: imx: regression triggered by newly introduced DSR irq handling

In reply to Uwe Kleine-König

Hey Uwe

Quoted 1 line · Uwe Kleine-K?nig

Once this is done (and still an issue)

Yes, patch b5ca028fe9ba2090a (ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1) is done and mainline and DSR irq handling is still an issue.

Quoted 4 lines

I'd suggest to standardize a dt
property

	disable-dsr;

Would you go forward?

Quoted 2 lines

(and the same for the other handshaking lines) and support th{is,ese} in the imx uart driver. Reverting 27e16501052e IMO isn't a sensible option.

Okay, I'm fine with a 'disable-dsr' solution.

Thanks
-- Christoph

author
#19 lore raw

serial: imx: regression triggered by newly introduced DSR irq handling

Hello Christoph,

Quoted 7 lines · Christoph Fritz
Quoted 1 line · Uwe Kleine-K?nig

Once this is done (and still an issue)

Yes, patch b5ca028fe9ba2090a (ARM: dts: imx6sx: document SION necessity of ENET1_REF_CLK1) is done and mainline and DSR irq handling is still an issue.

Quoted 6 lines
Quoted 4 lines

I'd suggest to standardize a dt
property

	disable-dsr;

Would you go forward?

Currently I don't have the need and time to do this myself. So if you want to propose a patch and send if for review to the dt and serial MLs that would be a good next step. If you add me to Cc (or To) I can take a look.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#20 lore raw

v2 cover serial: imx: handshaking fixes and improvments

In reply to Uwe Kleine-König

Uwe Kleine-K?nig <[email protected]> [2016-03-24 14:24:19]:

Quoted 3 lines

Among others it made my i.MX25 stuck when the other side toggles DCD. The first 4 patches are fixes that I'd like to see in 4.6-rcX, the last two patches are merge window material.

Hi,

just FYI, my i.MX6 board has stopped booting over NFS on 4.6-rc1 and I've just bisected it down to the commit "serial: imx: repair and complete handshaking" 90ebc48386, it's leading to the following CPU stall:

[   72.084538] INFO: rcu_sched self-detected stall on CPU
[   72.089719]  0-...: (1 GPs behind) idle=9bd/2/0 softirq=97/100 fqs=5996
[   72.096425]   (t=6000 jiffies g=-239 c=-240 q=47)
[   72.101154] Task dump for CPU 0:
[   72.104389] swapper/0       R running      0     0      0 0x00000002
[   72.110778] Backtrace:
[   72.113276] [<8010a580>] (dump_backtrace) from [<8010a778>] (show_stack+0x18/0x1c)
[   72.120850]  r7:80a09bc0 r6:80a02458 r5:80a05c00 r4:00000000
[   72.126589] [<8010a760>] (show_stack) from [<80137ee0>] (sched_show_task+0xb8/0xd8)
[   72.134258] [<80137e28>] (sched_show_task) from [<80139780>] (dump_cpu_task+0x34/0x44)
[   72.142177]  r5:00000000 r4:00000000
[   72.145806] [<8013974c>] (dump_cpu_task) from [<80157de8>] (rcu_dump_cpu_stacks+0x7c/0xa0)
[   72.154072]  r5:00000000 r4:80a09bc0
[   72.157697] [<80157d6c>] (rcu_dump_cpu_stacks) from [<8015ae74>] (rcu_check_callbacks+0x218/0x65c)
[   72.166661]  r9:80a024a0 r8:0000002f r7:80a09bc0 r6:80a02100 r5:ef1a61c0 r4:80a09bc0
[   72.174499] [<8015ac5c>] (rcu_check_callbacks) from [<8015d0d0>] (update_process_times+0x38/0x68)
[   72.183376]  r10:ffffffff r9:ef1a2b40 r8:80a2d567 r7:00000010 r6:c84d9643 r5:00000000
[   72.191283]  r4:80a05c00
[   72.193854] [<8015d098>] (update_process_times) from [<8016c6bc>] (tick_sched_handle+0x50/0x54)
[   72.202555]  r5:80a01e20 r4:ef1a2cb8
[   72.206177] [<8016c66c>] (tick_sched_handle) from [<8016c70c>] (tick_sched_timer+0x4c/0x8c)
[   72.214546] [<8016c6c0>] (tick_sched_timer) from [<8015da94>] (__hrtimer_run_queues+0xd0/0x17c)
[   72.223247]  r7:00000001 r6:ef1a2b4c r5:ef1a2cb8 r4:ef1a2b00
[   72.228979] [<8015d9c4>] (__hrtimer_run_queues) from [<8015e074>] (hrtimer_interrupt+0xb8/0x208)
[   72.237768]  r10:ffffffff r9:7fffffff r8:00000003 r7:6e858000 r6:80a01d48 r5:8094ab00
[   72.237768]  r10:ffffffff r9:7fffffff r8:00000003 r7:6e858000 r6:80a01d48 r5:8094ab00
[   72.245674]  r4:ef1a2b00
[   72.248240] [<8015dfbc>] (hrtimer_interrupt) from [<8010dadc>] (twd_handler+0x34/0x40)
[   72.256159]  r10:80a01e70 r9:8093da28 r8:00000010 r7:eec1cc40 r6:80a026dc r5:eec03c00
[   72.264067]  r4:00000001
[   72.266629] [<8010daa8>] (twd_handler) from [<80152ef4>] (handle_percpu_devid_irq+0x70/0x8c)
[   72.275069]  r5:eec03c00 r4:ef1a8840
[   72.278695] [<80152e84>] (handle_percpu_devid_irq) from [<8014ec68>] (generic_handle_irq+0x20/0x30)
[   72.287744]  r9:8093da28 r8:eec05000 r7:00000010 r6:8094b044 r5:80a01f28 r4:00000000
[   72.295579] [<8014ec48>] (generic_handle_irq) from [<8014ef80>] (__handle_domain_irq+0x94/0xbc)
[   72.304292] [<8014eeec>] (__handle_domain_irq) from [<80101408>] (gic_handle_irq+0x58/0x98)
[   72.312646]  r9:8093da28 r8:f4001100 r7:80a11c50 r6:80a01e20 r5:80a026dc r4:f4000100
[   72.320482] [<801013b0>] (gic_handle_irq) from [<8010b214>] (__irq_svc+0x54/0x70)
[   72.327971] Exception stack(0x80a01e20 to 0x80a01e68)
[   72.333036] 1e20: 00000000 00200000 00000000 00000000 80a00000 00000000 00000282 80a2e100
[   72.341227] 1e40: eec05000 8093da28 80a01e70 80a01ec4 80a01ec8 80a01e70 8011a2bc 80119ea0
[   72.349409] 1e60: 60000113 ffffffff
[   72.352902]  r9:8093da28 r8:eec05000 r7:80a01e54 r6:ffffffff r5:60000113 r4:80119ea0
[   72.360748] [<80119e2c>] (__do_softirq) from [<8011a2bc>] (irq_exit+0x8c/0xfc)
[   72.367974]  r10:00000000 r9:8093da28 r8:eec05000 r7:00000010 r6:8094b044 r5:00000000
[   72.375880]  r4:00000000
[   72.378446] [<8011a230>] (irq_exit) from [<8014ef84>] (__handle_domain_irq+0x98/0xbc)
[   72.386290] [<8014eeec>] (__handle_domain_irq) from [<80101408>] (gic_handle_irq+0x58/0x98)
[   72.394644]  r9:8093da28 r8:f4001100 r7:80a11c50 r6:80a01f28 r5:80a026dc r4:f4000100
[   72.402476] [<801013b0>] (gic_handle_irq) from [<8010b214>] (__irq_svc+0x54/0x70)
[   72.409964] Exception stack(0x80a01f28 to 0x80a01f70)
[   72.415027] 1f20:                   00000000 000012ce ef1a227c 80112b00 80a00000 80a02430
[   72.423215] 1f40: 80a02430 80a02380 efffc840 8093da28 00000000 80a01f84 80a01f88 80a01f78
[   72.431400] 1f60: 801079a4 801079a8 60000013 ffffffff
[   72.436455]  r9:8093da28 r8:efffc840 r7:80a01f5c r6:ffffffff r5:60000013 r4:801079a8
[   72.444289] [<80107974>] (arch_cpu_idle) from [<80148174>] (default_idle_call+0x30/0x34)
[   72.452395] [<80148144>] (default_idle_call) from [<8014826c>] (cpu_startup_entry+0xf4/0x15c)
[   72.460936] [<80148178>] (cpu_startup_entry) from [<806294a0>] (rest_init+0x68/0x80)
[   72.468699] [<80629438>] (rest_init) from [<80900c74>] (start_kernel+0x2fc/0x368)
[   72.476193] [<80900978>] (start_kernel) from [<1000807c>] (0x1000807c)
[   72.482724]  r9:412fc09a r8:1000406a r7:80a06c7c r6:8093da24 r5:80a023dc r4:80a2da14

To fix this problem, I've to revert the 90ebc48386 commit or apply this patch series.

-- ynezz

author
#21 lore raw

v2 cover serial: imx: handshaking fixes and improvments

Hello Petr,

Quoted 9 lines · Petr ?tetiar

Uwe Kleine-K?nig <[email protected]> [2016-03-24 14:24:19]:

Quoted 3 lines

Among others it made my i.MX25 stuck when the other side toggles DCD. The first 4 patches are fixes that I'd like to see in 4.6-rcX, the last two patches are merge window material.

just FYI, my i.MX6 board has stopped booting over NFS on 4.6-rc1 and I've just bisected it down to the commit "serial: imx: repair and complete handshaking" 90ebc48386, it's leading to the following CPU stall:

hmm, I'm confused (because I don't understand it) and something between happy and sad (because both 90ebc48386 and the series that fixes it are mine).

This is an i.MX6 Solo (i.e. only one cpu)?

Quoted 62 lines
[   72.084538] INFO: rcu_sched self-detected stall on CPU
[   72.089719]  0-...: (1 GPs behind) idle=9bd/2/0 softirq=97/100 fqs=5996
[   72.096425]   (t=6000 jiffies g=-239 c=-240 q=47)
[   72.101154] Task dump for CPU 0:
[   72.104389] swapper/0       R running      0     0      0 0x00000002
[   72.110778] Backtrace:
[   72.113276] [<8010a580>] (dump_backtrace) from [<8010a778>] (show_stack+0x18/0x1c)
[   72.120850]  r7:80a09bc0 r6:80a02458 r5:80a05c00 r4:00000000
[   72.126589] [<8010a760>] (show_stack) from [<80137ee0>] (sched_show_task+0xb8/0xd8)
[   72.134258] [<80137e28>] (sched_show_task) from [<80139780>] (dump_cpu_task+0x34/0x44)
[   72.142177]  r5:00000000 r4:00000000
[   72.145806] [<8013974c>] (dump_cpu_task) from [<80157de8>] (rcu_dump_cpu_stacks+0x7c/0xa0)
[   72.154072]  r5:00000000 r4:80a09bc0
[   72.157697] [<80157d6c>] (rcu_dump_cpu_stacks) from [<8015ae74>] (rcu_check_callbacks+0x218/0x65c)
[   72.166661]  r9:80a024a0 r8:0000002f r7:80a09bc0 r6:80a02100 r5:ef1a61c0 r4:80a09bc0
[   72.174499] [<8015ac5c>] (rcu_check_callbacks) from [<8015d0d0>] (update_process_times+0x38/0x68)
[   72.183376]  r10:ffffffff r9:ef1a2b40 r8:80a2d567 r7:00000010 r6:c84d9643 r5:00000000
[   72.191283]  r4:80a05c00
[   72.193854] [<8015d098>] (update_process_times) from [<8016c6bc>] (tick_sched_handle+0x50/0x54)
[   72.202555]  r5:80a01e20 r4:ef1a2cb8
[   72.206177] [<8016c66c>] (tick_sched_handle) from [<8016c70c>] (tick_sched_timer+0x4c/0x8c)
[   72.214546] [<8016c6c0>] (tick_sched_timer) from [<8015da94>] (__hrtimer_run_queues+0xd0/0x17c)
[   72.223247]  r7:00000001 r6:ef1a2b4c r5:ef1a2cb8 r4:ef1a2b00
[   72.228979] [<8015d9c4>] (__hrtimer_run_queues) from [<8015e074>] (hrtimer_interrupt+0xb8/0x208)
[   72.237768]  r10:ffffffff r9:7fffffff r8:00000003 r7:6e858000 r6:80a01d48 r5:8094ab00
[   72.237768]  r10:ffffffff r9:7fffffff r8:00000003 r7:6e858000 r6:80a01d48 r5:8094ab00
[   72.245674]  r4:ef1a2b00
[   72.248240] [<8015dfbc>] (hrtimer_interrupt) from [<8010dadc>] (twd_handler+0x34/0x40)
[   72.256159]  r10:80a01e70 r9:8093da28 r8:00000010 r7:eec1cc40 r6:80a026dc r5:eec03c00
[   72.264067]  r4:00000001
[   72.266629] [<8010daa8>] (twd_handler) from [<80152ef4>] (handle_percpu_devid_irq+0x70/0x8c)
[   72.275069]  r5:eec03c00 r4:ef1a8840
[   72.278695] [<80152e84>] (handle_percpu_devid_irq) from [<8014ec68>] (generic_handle_irq+0x20/0x30)
[   72.287744]  r9:8093da28 r8:eec05000 r7:00000010 r6:8094b044 r5:80a01f28 r4:00000000
[   72.295579] [<8014ec48>] (generic_handle_irq) from [<8014ef80>] (__handle_domain_irq+0x94/0xbc)
[   72.304292] [<8014eeec>] (__handle_domain_irq) from [<80101408>] (gic_handle_irq+0x58/0x98)
[   72.312646]  r9:8093da28 r8:f4001100 r7:80a11c50 r6:80a01e20 r5:80a026dc r4:f4000100
[   72.320482] [<801013b0>] (gic_handle_irq) from [<8010b214>] (__irq_svc+0x54/0x70)
[   72.327971] Exception stack(0x80a01e20 to 0x80a01e68)
[   72.333036] 1e20: 00000000 00200000 00000000 00000000 80a00000 00000000 00000282 80a2e100
[   72.341227] 1e40: eec05000 8093da28 80a01e70 80a01ec4 80a01ec8 80a01e70 8011a2bc 80119ea0
[   72.349409] 1e60: 60000113 ffffffff
[   72.352902]  r9:8093da28 r8:eec05000 r7:80a01e54 r6:ffffffff r5:60000113 r4:80119ea0
[   72.360748] [<80119e2c>] (__do_softirq) from [<8011a2bc>] (irq_exit+0x8c/0xfc)
[   72.367974]  r10:00000000 r9:8093da28 r8:eec05000 r7:00000010 r6:8094b044 r5:00000000
[   72.375880]  r4:00000000
[   72.378446] [<8011a230>] (irq_exit) from [<8014ef84>] (__handle_domain_irq+0x98/0xbc)
[   72.386290] [<8014eeec>] (__handle_domain_irq) from [<80101408>] (gic_handle_irq+0x58/0x98)
[   72.394644]  r9:8093da28 r8:f4001100 r7:80a11c50 r6:80a01f28 r5:80a026dc r4:f4000100
[   72.402476] [<801013b0>] (gic_handle_irq) from [<8010b214>] (__irq_svc+0x54/0x70)
[   72.409964] Exception stack(0x80a01f28 to 0x80a01f70)
[   72.415027] 1f20:                   00000000 000012ce ef1a227c 80112b00 80a00000 80a02430
[   72.423215] 1f40: 80a02430 80a02380 efffc840 8093da28 00000000 80a01f84 80a01f88 80a01f78
[   72.431400] 1f60: 801079a4 801079a8 60000013 ffffffff
[   72.436455]  r9:8093da28 r8:efffc840 r7:80a01f5c r6:ffffffff r5:60000013 r4:801079a8
[   72.444289] [<80107974>] (arch_cpu_idle) from [<80148174>] (default_idle_call+0x30/0x34)
[   72.452395] [<80148144>] (default_idle_call) from [<8014826c>] (cpu_startup_entry+0xf4/0x15c)
[   72.460936] [<80148178>] (cpu_startup_entry) from [<806294a0>] (rest_init+0x68/0x80)
[   72.468699] [<80629438>] (rest_init) from [<80900c74>] (start_kernel+0x2fc/0x368)
[   72.476193] [<80900978>] (start_kernel) from [<1000807c>] (0x1000807c)
[   72.482724]  r9:412fc09a r8:1000406a r7:80a06c7c r6:8093da24 r5:80a023dc r4:80a2da14
Quoted 1 line

To fix this problem, I've to revert the 90ebc48386 commit or apply this patch series.

Which patch fixes your problem? Is it enough to revert just the last hunk of 90ebc48386? Do you use fsl,dte-mode for the uart in question?

IMHO patches 1-4 should get applied before 4.6.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#22 lore raw

v2 cover serial: imx: handshaking fixes and improvments

Uwe Kleine-K?nig <[email protected]> [2016-04-12 09:46:22]:

Hi,

Quoted 7 lines · Petr ?tetiar
Quoted 6 lines

Uwe Kleine-K?nig <[email protected]> [2016-03-24 14:24:19]:

Quoted 3 lines

Among others it made my i.MX25 stuck when the other side toggles DCD. The first 4 patches are fixes that I'd like to see in 4.6-rcX, the last two patches are merge window material.

Quoted 7 lines
Quoted 3 lines

just FYI, my i.MX6 board has stopped booting over NFS on 4.6-rc1 and I've just bisected it down to the commit "serial: imx: repair and complete handshaking" 90ebc48386, it's leading to the following CPU stall:

hmm, I'm confused (because I don't understand it) and something between happy and sad (because both 90ebc48386 and the series that fixes it are mine).

I don't know what's going on, probably something related to the serial IRQs?

Quoted 1 line

This is an i.MX6 Solo (i.e. only one cpu)?

This is i.MX6 Quad, Toradex Apalis Ixora (imx6q-apalis-ixora.dts).

Quoted 1 line

Which patch fixes your problem?

First 3 patches:

[v2,1/6] serial: imx: fix polarity of RI
[v2,2/6] serial: imx: let irq handler return IRQ_NONE if no event was handled [v2,3/6] serial: imx: make sure unhandled irqs are disabled

Quoted 1 line

Is it enough to revert just the last hunk of 90ebc48386?

Yes, this fixes it also:

	diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
	index 231e7d5..dab081c 100644
	--- a/drivers/tty/serial/imx.c
	+++ b/drivers/tty/serial/imx.c
	@@ -827,11 +827,6 @@ static void imx_set_mctrl(struct uart_port *port, unsigned int mctrl)
			writel(temp, sport->port.membase + UCR2);
		}
	 
	-       temp = readl(sport->port.membase + UCR3) & ~UCR3_DSR;
	-       if (!(mctrl & TIOCM_DTR))
	-               temp |= UCR3_DSR;
	-       writel(temp, sport->port.membase + UCR3);
	-
		temp = readl(sport->port.membase + uts_reg(sport)) & ~UTS_LOOP;
		if (mctrl & TIOCM_LOOP)
			temp |= UTS_LOOP;
Quoted 1 line

Do you use fsl,dte-mode for the uart in question?

Yes, from imx6qdl-apalis.dtsi:

	&uart1 {
		pinctrl-names = "default";
		pinctrl-0 = <&pinctrl_uart1_dte &pinctrl_uart1_ctrl>;
		fsl,dte-mode;
		fsl,uart-has-rtscts;
		status = "disabled";
	};
Quoted 1 line

IMHO patches 1-4 should get applied before 4.6.

Ok, good, but this leaves 4.5 broken, right? Thanks.

-- ynezz

author
#23 lore raw

v2 cover serial: imx: handshaking fixes and improvments

Hello Petr,

Quoted 55 lines · Petr ?tetiar

Uwe Kleine-K?nig <[email protected]> [2016-04-12 09:46:22]:

Quoted 7 lines · Petr ?tetiar
Quoted 6 lines

Uwe Kleine-K?nig <[email protected]> [2016-03-24 14:24:19]:

Quoted 3 lines

Among others it made my i.MX25 stuck when the other side toggles DCD. The first 4 patches are fixes that I'd like to see in 4.6-rcX, the last two patches are merge window material.

Quoted 7 lines
Quoted 3 lines

just FYI, my i.MX6 board has stopped booting over NFS on 4.6-rc1 and I've just bisected it down to the commit "serial: imx: repair and complete handshaking" 90ebc48386, it's leading to the following CPU stall:

hmm, I'm confused (because I don't understand it) and something between happy and sad (because both 90ebc48386 and the series that fixes it are mine).

I don't know what's going on, probably something related to the serial IRQs?

Quoted 1 line

This is an i.MX6 Solo (i.e. only one cpu)?

This is i.MX6 Quad, Toradex Apalis Ixora (imx6q-apalis-ixora.dts).

Quoted 1 line

Which patch fixes your problem?

First 3 patches:

[v2,1/6] serial: imx: fix polarity of RI
[v2,2/6] serial: imx: let irq handler return IRQ_NONE if no event was handled [v2,3/6] serial: imx: make sure unhandled irqs are disabled

Quoted 1 line

Is it enough to revert just the last hunk of 90ebc48386?

Yes, this fixes it also:

	diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
	index 231e7d5..dab081c 100644
	--- a/drivers/tty/serial/imx.c
	+++ b/drivers/tty/serial/imx.c
	@@ -827,11 +827,6 @@ static void imx_set_mctrl(struct uart_port *port, unsigned int mctrl)
			writel(temp, sport->port.membase + UCR2);
		}
	 
	-       temp = readl(sport->port.membase + UCR3) & ~UCR3_DSR;
	-       if (!(mctrl & TIOCM_DTR))
	-               temp |= UCR3_DSR;
	-       writel(temp, sport->port.membase + UCR3);
	-
		temp = readl(sport->port.membase + uts_reg(sport)) & ~UTS_LOOP;
		if (mctrl & TIOCM_LOOP)
			temp |= UTS_LOOP;
Quoted 1 line

Do you use fsl,dte-mode for the uart in question?

Yes, from imx6qdl-apalis.dtsi:

Quoted 7 lines
	&uart1 {
		pinctrl-names = "default";
		pinctrl-0 = <&pinctrl_uart1_dte &pinctrl_uart1_ctrl>;
		fsl,dte-mode;
		fsl,uart-has-rtscts;
		status = "disabled";
	};

ok, so UFCR_DCEDTE is set and UCR3_DSR controls the DTR output as intended. What is connected to the respective pin? MX6QDL_PAD_EIM_D25__UART1_DSR_B I think is the one.

Quoted 3 lines
Quoted 1 line

IMHO patches 1-4 should get applied before 4.6.

Ok, good, but this leaves 4.5 broken, right? Thanks.

Once they landed in 4.6-rc they can and probably should be backported for stable, yes.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#24 lore raw

v2 cover serial: imx: handshaking fixes and improvments

Uwe Kleine-K?nig <[email protected]> [2016-04-12 12:58:56]:

Quoted 3 lines

ok, so UFCR_DCEDTE is set and UCR3_DSR controls the DTR output as intended. What is connected to the respective pin? MX6QDL_PAD_EIM_D25__UART1_DSR_B I think is the one.

It's routed out to Apalis SoM pin 120, which is then connected via 22R resistor and through SN65C3243DBR RS-232 line driver to header which I use as debug/serial console.

-- ynezz

author
#25 lore raw

v2 cover serial: imx: handshaking fixes and improvments

Quoted 9 lines · Petr ?tetiar

Uwe Kleine-K?nig <[email protected]> [2016-04-12 12:58:56]:

Quoted 3 lines

ok, so UFCR_DCEDTE is set and UCR3_DSR controls the DTR output as intended. What is connected to the respective pin? MX6QDL_PAD_EIM_D25__UART1_DSR_B I think is the one.

It's routed out to Apalis SoM pin 120, which is then connected via 22R resistor and through SN65C3243DBR RS-232 line driver to header which I use as debug/serial console.

Then I guess the problem is that the rs232-partner asserts the imx6's DSR in reply to the imx6 asserting DTR?

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#26 lore raw

v2 cover serial: imx: handshaking fixes and improvments

Uwe Kleine-K?nig <[email protected]> [2016-04-12 19:30:32]:

Quoted 7 lines · Petr ?tetiar
Quoted 6 lines

Uwe Kleine-K?nig <[email protected]> [2016-04-12 12:58:56]:

Quoted 3 lines

ok, so UFCR_DCEDTE is set and UCR3_DSR controls the DTR output as intended. What is connected to the respective pin? MX6QDL_PAD_EIM_D25__UART1_DSR_B I think is the one.

Quoted 6 lines
Quoted 3 lines

It's routed out to Apalis SoM pin 120, which is then connected via 22R resistor and through SN65C3243DBR RS-232 line driver to header which I use as debug/serial console.

Then I guess the problem is that the rs232-partner asserts the imx6's DSR in reply to the imx6 asserting DTR?

Yes, that's correct.

-- ynezz

author
#27 lore raw

v2 cover serial: imx: handshaking fixes and improvments

Quoted 9 lines · Petr ?tetiar

Uwe Kleine-K?nig <[email protected]> [2016-04-12 19:30:32]:

Quoted 7 lines · Petr ?tetiar
Quoted 6 lines

Uwe Kleine-K?nig <[email protected]> [2016-04-12 12:58:56]:

Quoted 3 lines

ok, so UFCR_DCEDTE is set and UCR3_DSR controls the DTR output as intended. What is connected to the respective pin? MX6QDL_PAD_EIM_D25__UART1_DSR_B I think is the one.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

It's routed out to Apalis SoM pin 120, which is then connected via 22R resistor and through SN65C3243DBR RS-232 line driver to header which I use as debug/serial console.

Then I guess the problem is that the rs232-partner asserts the imx6's DSR in reply to the imx6 asserting DTR?

Yes, that's correct.

OK, that fits nicely in my picture. So we only have to wait until Greg happily picks up the fixes (i.e. patches 1 up to 4).

Best regards
Uwe

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