[PATCH] tty: serial: imx: Handle RS485 DE signal active high

[PATCH] tty: serial: imx: Handle RS485 DE signal active high

[Marek Vasut]

The default polarity of RS485 DE signal is active high. This driver does
not handle such case properly. Currently, when a pin is multiplexed as a
UART CTS_B on boot, this pin is pulled HIGH by the i.MX UART CTS circuit,
which activates DE signal on the RS485 transceiver and thus behave as if
the RS485 was transmitting data, so the system blocks the RS485 bus when
it starts and until user application takes over. This behavior is not OK.
The problem consists of two separate parts.

First, the i.MX UART IP requires UCR1 UARTEN and UCR2 RXEN to be set for
UCR2 CTSC and CTS bits to have any effect. The UCR2 CTSC bit permits the
driver to set CTS (RTS_B or RS485 DE signal) to either level sychronous
to the internal UART IP clock. Compared to other options, like GPIO CTS
control, this has the benefit of being synchronous to the UART IP clock
and thus without glitches or bus delays. The reason for the CTS design
is likely because when the Receiver is disabled, the UART IP can never
indicate that it is ready to receive data by assering CTS signal, so
the CTS is always pulled HIGH by default.

When the port is closed by user space, imx_uart_stop_rx() clears UCR2
RXEN bit, and imx_uart_shutdown() clears UCR1 UARTEN bit. This disables
UART Receiver and UART itself, and forces CTS signal HIGH, which leads
to the RS485 bus being blocked because RS485 DE is incorrectly active.

The proposed solution for this problem is to keep the Receiver running
even after the port is closed, but in loopback mode. This disconnects
the RX FIFO input from the RXD external signal, and since UCR2 TXEN is
cleared, the UART Transmitter is disabled, so nothing can feed data in
the RX FIFO. Because the Receiver is still enabled, the UCR2 CTSC and
CTS bits still have effect and the CTS (RS485 DE) control is retained.

Note that in case of RS485 DE signal active low, there is no problem and
no special handling is necessary. The CTS signal defaults to HIGH, thus
the RS485 is by default set to Receive and the bus is not blocked.

Note that while there is the possibility to control CTS using GPIO with
either CTS polarity, this has the downside of not being synchronous to
the UART IP clock and thus glitchy and susceptible to slow DE switching.

Second, on boot, before the UART driver probe callback is called, the
driver core triggers pinctrl_init_done() and configures the IOMUXC to
default state. At this point, UCR1 UARTEN and UCR2 RXEN are both still
cleared, but UART CTS_B (RS485 DE) is configured as CTS function, thus
the RTS signal is pulled HIGH by the UART IP CTS circuit.

One part of the solution here is to enable UCR1 UARTEN and UCR2 RXEN and
UTS loopback in this driver probe callback, thus unblocking the CTSC and
CTS control early on. But this is still too late, since the pin control
is already configured and CTS has been pulled HIGH for a short period
of time.

When Linux kernel boots and this driver is bound, the pin control is set
to special "init" state if the state is available, and driver can switch
the "default" state afterward when ready. This state can be used to set
the CTS line as a GPIO in DT temporarily, and a GPIO hog can force such
GPIO to LOW, thus keeping the RS485 DE line LOW early on boot. Once the
driver takes over and UCR1 UARTEN and UCR2 RXEN and UTS loopback are all
enabled, the driver can switch to "default" pin control state and control
the CTS line as function instead. DT binding example is below:

"
&gpio6 {
  rts-init-hog {
    gpio-hog;
    gpios = <5 0>;
    output-low;
    line-name = "rs485-de";
  };
};

&uart5 { /* DHCOM UART2 */
  pinctrl-0 = <&pinctrl_uart5>;
  pinctrl-1 = <&pinctrl_uart5_init>;
  pinctrl-names = "default", "init";
  ...
};
pinctrl_uart5_init: uart5-init-grp {
  fsl,pins = <
...
    MX6QDL_PAD_CSI0_DAT19__GPIO6_IO05       0x30b1
  >;
};

pinctrl_uart5: uart5-grp {
  fsl,pins = <
...
    MX6QDL_PAD_CSI0_DAT19__UART5_CTS_B      0x30b1
  >;
};
"

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Fabio Estevam <festevam@denx.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: kernel@dh-electronics.com
Cc: kernel@pengutronix.de
Cc: linux-arm-kernel@lists.infradead.org
To: linux-serial@vger.kernel.org
---
 drivers/tty/serial/imx.c | 51 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 05b432dc7a85c..144f1cedd4b64 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -489,7 +489,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
 static void imx_uart_stop_rx(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
-	u32 ucr1, ucr2, ucr4;
+	u32 ucr1, ucr2, ucr4, uts;
 
 	ucr1 = imx_uart_readl(sport, UCR1);
 	ucr2 = imx_uart_readl(sport, UCR2);
@@ -505,7 +505,17 @@ static void imx_uart_stop_rx(struct uart_port *port)
 	imx_uart_writel(sport, ucr1, UCR1);
 	imx_uart_writel(sport, ucr4, UCR4);
 
-	ucr2 &= ~UCR2_RXEN;
+	if (port->rs485.flags & SER_RS485_ENABLED &&
+	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
+	    sport->have_rtscts && !sport->have_rtsgpio) {
+		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+		uts |= UTS_LOOP;
+		imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+		ucr2 |= UCR2_RXEN;
+	} else {
+		ucr2 &= ~UCR2_RXEN;
+	}
+
 	imx_uart_writel(sport, ucr2, UCR2);
 }
 
@@ -1393,7 +1403,7 @@ static int imx_uart_startup(struct uart_port *port)
 	int retval, i;
 	unsigned long flags;
 	int dma_is_inited = 0;
-	u32 ucr1, ucr2, ucr3, ucr4;
+	u32 ucr1, ucr2, ucr3, ucr4, uts;
 
 	retval = clk_prepare_enable(sport->clk_per);
 	if (retval)
@@ -1498,6 +1508,10 @@ static int imx_uart_startup(struct uart_port *port)
 		imx_uart_writel(sport, ucr2, UCR2);
 	}
 
+	uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+	uts &= ~UTS_LOOP;
+	imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
 	return 0;
@@ -1507,7 +1521,7 @@ static void imx_uart_shutdown(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	unsigned long flags;
-	u32 ucr1, ucr2, ucr4;
+	u32 ucr1, ucr2, ucr4, uts;
 
 	if (sport->dma_is_enabled) {
 		dmaengine_terminate_sync(sport->dma_chan_tx);
@@ -1551,7 +1565,17 @@ static void imx_uart_shutdown(struct uart_port *port)
 	spin_lock_irqsave(&sport->port.lock, flags);
 
 	ucr1 = imx_uart_readl(sport, UCR1);
-	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
+	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
+	if (port->rs485.flags & SER_RS485_ENABLED &&
+	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
+	    sport->have_rtscts && !sport->have_rtsgpio) {
+		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+		uts |= UTS_LOOP;
+		imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+		ucr1 |= UCR1_UARTEN;
+	} else {
+		ucr1 &= ~UCR1_UARTEN;
+	}
 	imx_uart_writel(sport, ucr1, UCR1);
 
 	ucr4 = imx_uart_readl(sport, UCR4);
@@ -2213,7 +2237,7 @@ static int imx_uart_probe(struct platform_device *pdev)
 	void __iomem *base;
 	u32 dma_buf_conf[2];
 	int ret = 0;
-	u32 ucr1;
+	u32 ucr1, ucr2, uts;
 	struct resource *res;
 	int txirq, rxirq, rtsirq;
 
@@ -2350,6 +2374,21 @@ static int imx_uart_probe(struct platform_device *pdev)
 	ucr1 &= ~(UCR1_ADEN | UCR1_TRDYEN | UCR1_IDEN | UCR1_RRDYEN | UCR1_RTSDEN);
 	imx_uart_writel(sport, ucr1, UCR1);
 
+	if (sport->port.rs485.flags & SER_RS485_ENABLED &&
+	    sport->have_rtscts && !sport->have_rtsgpio) {
+		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+		uts |= UTS_LOOP;
+		imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+
+		ucr1 = imx_uart_readl(sport, UCR1);
+		ucr1 |= UCR1_UARTEN;
+		imx_uart_writel(sport, ucr1, UCR1);
+
+		ucr2 = imx_uart_readl(sport, UCR2);
+		ucr2 |= UCR2_RXEN;
+		imx_uart_writel(sport, ucr2, UCR2);
+	}
+
 	if (!imx_uart_is_imx1(sport) && sport->dte_mode) {
 		/*
 		 * The DCEDTE bit changes the direction of DSR, DCD, DTR and RI
-- 
2.35.1

[PATCH] tty: serial: imx: Handle RS485 DE signal active high

[Marek Vasut]

The default polarity of RS485 DE signal is active high. This driver does
not handle such case properly. Currently, when a pin is multiplexed as a
UART CTS_B on boot, this pin is pulled HIGH by the i.MX UART CTS circuit,
which activates DE signal on the RS485 transceiver and thus behave as if
the RS485 was transmitting data, so the system blocks the RS485 bus when
it starts and until user application takes over. This behavior is not OK.
The problem consists of two separate parts.

First, the i.MX UART IP requires UCR1 UARTEN and UCR2 RXEN to be set for
UCR2 CTSC and CTS bits to have any effect. The UCR2 CTSC bit permits the
driver to set CTS (RTS_B or RS485 DE signal) to either level sychronous
to the internal UART IP clock. Compared to other options, like GPIO CTS
control, this has the benefit of being synchronous to the UART IP clock
and thus without glitches or bus delays. The reason for the CTS design
is likely because when the Receiver is disabled, the UART IP can never
indicate that it is ready to receive data by assering CTS signal, so
the CTS is always pulled HIGH by default.

When the port is closed by user space, imx_uart_stop_rx() clears UCR2
RXEN bit, and imx_uart_shutdown() clears UCR1 UARTEN bit. This disables
UART Receiver and UART itself, and forces CTS signal HIGH, which leads
to the RS485 bus being blocked because RS485 DE is incorrectly active.

The proposed solution for this problem is to keep the Receiver running
even after the port is closed, but in loopback mode. This disconnects
the RX FIFO input from the RXD external signal, and since UCR2 TXEN is
cleared, the UART Transmitter is disabled, so nothing can feed data in
the RX FIFO. Because the Receiver is still enabled, the UCR2 CTSC and
CTS bits still have effect and the CTS (RS485 DE) control is retained.

Note that in case of RS485 DE signal active low, there is no problem and
no special handling is necessary. The CTS signal defaults to HIGH, thus
the RS485 is by default set to Receive and the bus is not blocked.

Note that while there is the possibility to control CTS using GPIO with
either CTS polarity, this has the downside of not being synchronous to
the UART IP clock and thus glitchy and susceptible to slow DE switching.

Second, on boot, before the UART driver probe callback is called, the
driver core triggers pinctrl_init_done() and configures the IOMUXC to
default state. At this point, UCR1 UARTEN and UCR2 RXEN are both still
cleared, but UART CTS_B (RS485 DE) is configured as CTS function, thus
the RTS signal is pulled HIGH by the UART IP CTS circuit.

One part of the solution here is to enable UCR1 UARTEN and UCR2 RXEN and
UTS loopback in this driver probe callback, thus unblocking the CTSC and
CTS control early on. But this is still too late, since the pin control
is already configured and CTS has been pulled HIGH for a short period
of time.

When Linux kernel boots and this driver is bound, the pin control is set
to special "init" state if the state is available, and driver can switch
the "default" state afterward when ready. This state can be used to set
the CTS line as a GPIO in DT temporarily, and a GPIO hog can force such
GPIO to LOW, thus keeping the RS485 DE line LOW early on boot. Once the
driver takes over and UCR1 UARTEN and UCR2 RXEN and UTS loopback are all
enabled, the driver can switch to "default" pin control state and control
the CTS line as function instead. DT binding example is below:

"
&gpio6 {
  rts-init-hog {
    gpio-hog;
    gpios = <5 0>;
    output-low;
    line-name = "rs485-de";
  };
};

&uart5 { /* DHCOM UART2 */
  pinctrl-0 = <&pinctrl_uart5>;
  pinctrl-1 = <&pinctrl_uart5_init>;
  pinctrl-names = "default", "init";
  ...
};
pinctrl_uart5_init: uart5-init-grp {
  fsl,pins = <
...
    MX6QDL_PAD_CSI0_DAT19__GPIO6_IO05       0x30b1
  >;
};

pinctrl_uart5: uart5-grp {
  fsl,pins = <
...
    MX6QDL_PAD_CSI0_DAT19__UART5_CTS_B      0x30b1
  >;
};
"

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Fabio Estevam <festevam@denx.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: kernel@dh-electronics.com
Cc: kernel@pengutronix.de
Cc: linux-arm-kernel@lists.infradead.org
To: linux-serial@vger.kernel.org
---
 drivers/tty/serial/imx.c | 51 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 05b432dc7a85c..144f1cedd4b64 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -489,7 +489,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
 static void imx_uart_stop_rx(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
-	u32 ucr1, ucr2, ucr4;
+	u32 ucr1, ucr2, ucr4, uts;
 
 	ucr1 = imx_uart_readl(sport, UCR1);
 	ucr2 = imx_uart_readl(sport, UCR2);
@@ -505,7 +505,17 @@ static void imx_uart_stop_rx(struct uart_port *port)
 	imx_uart_writel(sport, ucr1, UCR1);
 	imx_uart_writel(sport, ucr4, UCR4);
 
-	ucr2 &= ~UCR2_RXEN;
+	if (port->rs485.flags & SER_RS485_ENABLED &&
+	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
+	    sport->have_rtscts && !sport->have_rtsgpio) {
+		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+		uts |= UTS_LOOP;
+		imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+		ucr2 |= UCR2_RXEN;
+	} else {
+		ucr2 &= ~UCR2_RXEN;
+	}
+
 	imx_uart_writel(sport, ucr2, UCR2);
 }
 
@@ -1393,7 +1403,7 @@ static int imx_uart_startup(struct uart_port *port)
 	int retval, i;
 	unsigned long flags;
 	int dma_is_inited = 0;
-	u32 ucr1, ucr2, ucr3, ucr4;
+	u32 ucr1, ucr2, ucr3, ucr4, uts;
 
 	retval = clk_prepare_enable(sport->clk_per);
 	if (retval)
@@ -1498,6 +1508,10 @@ static int imx_uart_startup(struct uart_port *port)
 		imx_uart_writel(sport, ucr2, UCR2);
 	}
 
+	uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+	uts &= ~UTS_LOOP;
+	imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+
 	spin_unlock_irqrestore(&sport->port.lock, flags);
 
 	return 0;
@@ -1507,7 +1521,7 @@ static void imx_uart_shutdown(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	unsigned long flags;
-	u32 ucr1, ucr2, ucr4;
+	u32 ucr1, ucr2, ucr4, uts;
 
 	if (sport->dma_is_enabled) {
 		dmaengine_terminate_sync(sport->dma_chan_tx);
@@ -1551,7 +1565,17 @@ static void imx_uart_shutdown(struct uart_port *port)
 	spin_lock_irqsave(&sport->port.lock, flags);
 
 	ucr1 = imx_uart_readl(sport, UCR1);
-	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
+	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
+	if (port->rs485.flags & SER_RS485_ENABLED &&
+	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
+	    sport->have_rtscts && !sport->have_rtsgpio) {
+		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+		uts |= UTS_LOOP;
+		imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+		ucr1 |= UCR1_UARTEN;
+	} else {
+		ucr1 &= ~UCR1_UARTEN;
+	}
 	imx_uart_writel(sport, ucr1, UCR1);
 
 	ucr4 = imx_uart_readl(sport, UCR4);
@@ -2213,7 +2237,7 @@ static int imx_uart_probe(struct platform_device *pdev)
 	void __iomem *base;
 	u32 dma_buf_conf[2];
 	int ret = 0;
-	u32 ucr1;
+	u32 ucr1, ucr2, uts;
 	struct resource *res;
 	int txirq, rxirq, rtsirq;
 
@@ -2350,6 +2374,21 @@ static int imx_uart_probe(struct platform_device *pdev)
 	ucr1 &= ~(UCR1_ADEN | UCR1_TRDYEN | UCR1_IDEN | UCR1_RRDYEN | UCR1_RTSDEN);
 	imx_uart_writel(sport, ucr1, UCR1);
 
+	if (sport->port.rs485.flags & SER_RS485_ENABLED &&
+	    sport->have_rtscts && !sport->have_rtsgpio) {
+		uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
+		uts |= UTS_LOOP;
+		imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
+
+		ucr1 = imx_uart_readl(sport, UCR1);
+		ucr1 |= UCR1_UARTEN;
+		imx_uart_writel(sport, ucr1, UCR1);
+
+		ucr2 = imx_uart_readl(sport, UCR2);
+		ucr2 |= UCR2_RXEN;
+		imx_uart_writel(sport, ucr2, UCR2);
+	}
+
 	if (!imx_uart_is_imx1(sport) && sport->dte_mode) {
 		/*
 		 * The DCEDTE bit changes the direction of DSR, DCD, DTR and RI
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: serial: imx: Handle RS485 DE signal active high

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 6276 bytes --]

On Wed, Sep 28, 2022 at 06:40:37AM +0200, Marek Vasut wrote:
> The default polarity of RS485 DE signal is active high. This driver does
> not handle such case properly. Currently, when a pin is multiplexed as a
> UART CTS_B on boot, this pin is pulled HIGH by the i.MX UART CTS circuit,
> which activates DE signal on the RS485 transceiver and thus behave as if
> the RS485 was transmitting data, so the system blocks the RS485 bus when
> it starts and until user application takes over. This behavior is not OK.
> The problem consists of two separate parts.
> 
> First, the i.MX UART IP requires UCR1 UARTEN and UCR2 RXEN to be set for
> UCR2 CTSC and CTS bits to have any effect. The UCR2 CTSC bit permits the
> driver to set CTS (RTS_B or RS485 DE signal) to either level sychronous
> to the internal UART IP clock. Compared to other options, like GPIO CTS
> control, this has the benefit of being synchronous to the UART IP clock
> and thus without glitches or bus delays. The reason for the CTS design
> is likely because when the Receiver is disabled, the UART IP can never
> indicate that it is ready to receive data by assering CTS signal, so
> the CTS is always pulled HIGH by default.
> 
> When the port is closed by user space, imx_uart_stop_rx() clears UCR2
> RXEN bit, and imx_uart_shutdown() clears UCR1 UARTEN bit. This disables
> UART Receiver and UART itself, and forces CTS signal HIGH, which leads
> to the RS485 bus being blocked because RS485 DE is incorrectly active.
> 
> The proposed solution for this problem is to keep the Receiver running
> even after the port is closed, but in loopback mode. This disconnects
> the RX FIFO input from the RXD external signal, and since UCR2 TXEN is
> cleared, the UART Transmitter is disabled, so nothing can feed data in
> the RX FIFO. Because the Receiver is still enabled, the UCR2 CTSC and
> CTS bits still have effect and the CTS (RS485 DE) control is retained.
> 
> Note that in case of RS485 DE signal active low, there is no problem and
> no special handling is necessary. The CTS signal defaults to HIGH, thus
> the RS485 is by default set to Receive and the bus is not blocked.
> 
> Note that while there is the possibility to control CTS using GPIO with
> either CTS polarity, this has the downside of not being synchronous to
> the UART IP clock and thus glitchy and susceptible to slow DE switching.
> 
> Second, on boot, before the UART driver probe callback is called, the
> driver core triggers pinctrl_init_done() and configures the IOMUXC to
> default state. At this point, UCR1 UARTEN and UCR2 RXEN are both still
> cleared, but UART CTS_B (RS485 DE) is configured as CTS function, thus
> the RTS signal is pulled HIGH by the UART IP CTS circuit.
> 
> One part of the solution here is to enable UCR1 UARTEN and UCR2 RXEN and
> UTS loopback in this driver probe callback, thus unblocking the CTSC and
> CTS control early on. But this is still too late, since the pin control
> is already configured and CTS has been pulled HIGH for a short period
> of time.
> 
> When Linux kernel boots and this driver is bound, the pin control is set
> to special "init" state if the state is available, and driver can switch
> the "default" state afterward when ready. This state can be used to set
> the CTS line as a GPIO in DT temporarily, and a GPIO hog can force such
> GPIO to LOW, thus keeping the RS485 DE line LOW early on boot. Once the
> driver takes over and UCR1 UARTEN and UCR2 RXEN and UTS loopback are all
> enabled, the driver can switch to "default" pin control state and control
> the CTS line as function instead. DT binding example is below:
> 
> "
> &gpio6 {
>   rts-init-hog {
>     gpio-hog;
>     gpios = <5 0>;
>     output-low;
>     line-name = "rs485-de";
>   };
> };
> 
> &uart5 { /* DHCOM UART2 */
>   pinctrl-0 = <&pinctrl_uart5>;
>   pinctrl-1 = <&pinctrl_uart5_init>;
>   pinctrl-names = "default", "init";
>   ...
> };
> pinctrl_uart5_init: uart5-init-grp {
>   fsl,pins = <
> ...
>     MX6QDL_PAD_CSI0_DAT19__GPIO6_IO05       0x30b1
>   >;
> };
> 
> pinctrl_uart5: uart5-grp {
>   fsl,pins = <
> ...
>     MX6QDL_PAD_CSI0_DAT19__UART5_CTS_B      0x30b1
>   >;
> };
> "

An alternative work around is to use the GPIO as RTS also in default,
isn't it?

> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Fabio Estevam <festevam@denx.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: kernel@dh-electronics.com
> Cc: kernel@pengutronix.de
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/imx.c | 51 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 05b432dc7a85c..144f1cedd4b64 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -489,7 +489,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
>  static void imx_uart_stop_rx(struct uart_port *port)
>  {
>  	struct imx_port *sport = (struct imx_port *)port;
> -	u32 ucr1, ucr2, ucr4;
> +	u32 ucr1, ucr2, ucr4, uts;
>  
>  	ucr1 = imx_uart_readl(sport, UCR1);
>  	ucr2 = imx_uart_readl(sport, UCR2);
> @@ -505,7 +505,17 @@ static void imx_uart_stop_rx(struct uart_port *port)
>  	imx_uart_writel(sport, ucr1, UCR1);
>  	imx_uart_writel(sport, ucr4, UCR4);
>  
> -	ucr2 &= ~UCR2_RXEN;
> +	if (port->rs485.flags & SER_RS485_ENABLED &&
> +	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
> +	    sport->have_rtscts && !sport->have_rtsgpio) {

IMHO this needs a comment. The later hunks might benefit from a comment
two. I'd explain it once in more detail and refer to that description
from the other relevant code sections.

Best regards
Uwe

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] tty: serial: imx: Handle RS485 DE signal active high

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 6276 bytes --]

On Wed, Sep 28, 2022 at 06:40:37AM +0200, Marek Vasut wrote:
> The default polarity of RS485 DE signal is active high. This driver does
> not handle such case properly. Currently, when a pin is multiplexed as a
> UART CTS_B on boot, this pin is pulled HIGH by the i.MX UART CTS circuit,
> which activates DE signal on the RS485 transceiver and thus behave as if
> the RS485 was transmitting data, so the system blocks the RS485 bus when
> it starts and until user application takes over. This behavior is not OK.
> The problem consists of two separate parts.
> 
> First, the i.MX UART IP requires UCR1 UARTEN and UCR2 RXEN to be set for
> UCR2 CTSC and CTS bits to have any effect. The UCR2 CTSC bit permits the
> driver to set CTS (RTS_B or RS485 DE signal) to either level sychronous
> to the internal UART IP clock. Compared to other options, like GPIO CTS
> control, this has the benefit of being synchronous to the UART IP clock
> and thus without glitches or bus delays. The reason for the CTS design
> is likely because when the Receiver is disabled, the UART IP can never
> indicate that it is ready to receive data by assering CTS signal, so
> the CTS is always pulled HIGH by default.
> 
> When the port is closed by user space, imx_uart_stop_rx() clears UCR2
> RXEN bit, and imx_uart_shutdown() clears UCR1 UARTEN bit. This disables
> UART Receiver and UART itself, and forces CTS signal HIGH, which leads
> to the RS485 bus being blocked because RS485 DE is incorrectly active.
> 
> The proposed solution for this problem is to keep the Receiver running
> even after the port is closed, but in loopback mode. This disconnects
> the RX FIFO input from the RXD external signal, and since UCR2 TXEN is
> cleared, the UART Transmitter is disabled, so nothing can feed data in
> the RX FIFO. Because the Receiver is still enabled, the UCR2 CTSC and
> CTS bits still have effect and the CTS (RS485 DE) control is retained.
> 
> Note that in case of RS485 DE signal active low, there is no problem and
> no special handling is necessary. The CTS signal defaults to HIGH, thus
> the RS485 is by default set to Receive and the bus is not blocked.
> 
> Note that while there is the possibility to control CTS using GPIO with
> either CTS polarity, this has the downside of not being synchronous to
> the UART IP clock and thus glitchy and susceptible to slow DE switching.
> 
> Second, on boot, before the UART driver probe callback is called, the
> driver core triggers pinctrl_init_done() and configures the IOMUXC to
> default state. At this point, UCR1 UARTEN and UCR2 RXEN are both still
> cleared, but UART CTS_B (RS485 DE) is configured as CTS function, thus
> the RTS signal is pulled HIGH by the UART IP CTS circuit.
> 
> One part of the solution here is to enable UCR1 UARTEN and UCR2 RXEN and
> UTS loopback in this driver probe callback, thus unblocking the CTSC and
> CTS control early on. But this is still too late, since the pin control
> is already configured and CTS has been pulled HIGH for a short period
> of time.
> 
> When Linux kernel boots and this driver is bound, the pin control is set
> to special "init" state if the state is available, and driver can switch
> the "default" state afterward when ready. This state can be used to set
> the CTS line as a GPIO in DT temporarily, and a GPIO hog can force such
> GPIO to LOW, thus keeping the RS485 DE line LOW early on boot. Once the
> driver takes over and UCR1 UARTEN and UCR2 RXEN and UTS loopback are all
> enabled, the driver can switch to "default" pin control state and control
> the CTS line as function instead. DT binding example is below:
> 
> "
> &gpio6 {
>   rts-init-hog {
>     gpio-hog;
>     gpios = <5 0>;
>     output-low;
>     line-name = "rs485-de";
>   };
> };
> 
> &uart5 { /* DHCOM UART2 */
>   pinctrl-0 = <&pinctrl_uart5>;
>   pinctrl-1 = <&pinctrl_uart5_init>;
>   pinctrl-names = "default", "init";
>   ...
> };
> pinctrl_uart5_init: uart5-init-grp {
>   fsl,pins = <
> ...
>     MX6QDL_PAD_CSI0_DAT19__GPIO6_IO05       0x30b1
>   >;
> };
> 
> pinctrl_uart5: uart5-grp {
>   fsl,pins = <
> ...
>     MX6QDL_PAD_CSI0_DAT19__UART5_CTS_B      0x30b1
>   >;
> };
> "

An alternative work around is to use the GPIO as RTS also in default,
isn't it?

> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Fabio Estevam <festevam@denx.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: kernel@dh-electronics.com
> Cc: kernel@pengutronix.de
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/imx.c | 51 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 05b432dc7a85c..144f1cedd4b64 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -489,7 +489,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
>  static void imx_uart_stop_rx(struct uart_port *port)
>  {
>  	struct imx_port *sport = (struct imx_port *)port;
> -	u32 ucr1, ucr2, ucr4;
> +	u32 ucr1, ucr2, ucr4, uts;
>  
>  	ucr1 = imx_uart_readl(sport, UCR1);
>  	ucr2 = imx_uart_readl(sport, UCR2);
> @@ -505,7 +505,17 @@ static void imx_uart_stop_rx(struct uart_port *port)
>  	imx_uart_writel(sport, ucr1, UCR1);
>  	imx_uart_writel(sport, ucr4, UCR4);
>  
> -	ucr2 &= ~UCR2_RXEN;
> +	if (port->rs485.flags & SER_RS485_ENABLED &&
> +	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
> +	    sport->have_rtscts && !sport->have_rtsgpio) {

IMHO this needs a comment. The later hunks might benefit from a comment
two. I'd explain it once in more detail and refer to that description
from the other relevant code sections.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] tty: serial: imx: Handle RS485 DE signal active high

[Marek Vasut]

On 9/28/22 09:43, Uwe Kleine-König wrote:
> On Wed, Sep 28, 2022 at 06:40:37AM +0200, Marek Vasut wrote:
>> The default polarity of RS485 DE signal is active high. This driver does
>> not handle such case properly. Currently, when a pin is multiplexed as a
>> UART CTS_B on boot, this pin is pulled HIGH by the i.MX UART CTS circuit,
>> which activates DE signal on the RS485 transceiver and thus behave as if
>> the RS485 was transmitting data, so the system blocks the RS485 bus when
>> it starts and until user application takes over. This behavior is not OK.
>> The problem consists of two separate parts.
>>
>> First, the i.MX UART IP requires UCR1 UARTEN and UCR2 RXEN to be set for
>> UCR2 CTSC and CTS bits to have any effect. The UCR2 CTSC bit permits the
>> driver to set CTS (RTS_B or RS485 DE signal) to either level sychronous
>> to the internal UART IP clock. Compared to other options, like GPIO CTS
>> control, this has the benefit of being synchronous to the UART IP clock
>> and thus without glitches or bus delays. The reason for the CTS design
>> is likely because when the Receiver is disabled, the UART IP can never
>> indicate that it is ready to receive data by assering CTS signal, so
>> the CTS is always pulled HIGH by default.
>>
>> When the port is closed by user space, imx_uart_stop_rx() clears UCR2
>> RXEN bit, and imx_uart_shutdown() clears UCR1 UARTEN bit. This disables
>> UART Receiver and UART itself, and forces CTS signal HIGH, which leads
>> to the RS485 bus being blocked because RS485 DE is incorrectly active.
>>
>> The proposed solution for this problem is to keep the Receiver running
>> even after the port is closed, but in loopback mode. This disconnects
>> the RX FIFO input from the RXD external signal, and since UCR2 TXEN is
>> cleared, the UART Transmitter is disabled, so nothing can feed data in
>> the RX FIFO. Because the Receiver is still enabled, the UCR2 CTSC and
>> CTS bits still have effect and the CTS (RS485 DE) control is retained.
>>
>> Note that in case of RS485 DE signal active low, there is no problem and
>> no special handling is necessary. The CTS signal defaults to HIGH, thus
>> the RS485 is by default set to Receive and the bus is not blocked.
>>
>> Note that while there is the possibility to control CTS using GPIO with
>> either CTS polarity, this has the downside of not being synchronous to
>> the UART IP clock and thus glitchy and susceptible to slow DE switching.
>>
>> Second, on boot, before the UART driver probe callback is called, the
>> driver core triggers pinctrl_init_done() and configures the IOMUXC to
>> default state. At this point, UCR1 UARTEN and UCR2 RXEN are both still
>> cleared, but UART CTS_B (RS485 DE) is configured as CTS function, thus
>> the RTS signal is pulled HIGH by the UART IP CTS circuit.
>>
>> One part of the solution here is to enable UCR1 UARTEN and UCR2 RXEN and
>> UTS loopback in this driver probe callback, thus unblocking the CTSC and
>> CTS control early on. But this is still too late, since the pin control
>> is already configured and CTS has been pulled HIGH for a short period
>> of time.
>>
>> When Linux kernel boots and this driver is bound, the pin control is set
>> to special "init" state if the state is available, and driver can switch
>> the "default" state afterward when ready. This state can be used to set
>> the CTS line as a GPIO in DT temporarily, and a GPIO hog can force such
>> GPIO to LOW, thus keeping the RS485 DE line LOW early on boot. Once the
>> driver takes over and UCR1 UARTEN and UCR2 RXEN and UTS loopback are all
>> enabled, the driver can switch to "default" pin control state and control
>> the CTS line as function instead. DT binding example is below:
>>
>> "
>> &gpio6 {
>>    rts-init-hog {
>>      gpio-hog;
>>      gpios = <5 0>;
>>      output-low;
>>      line-name = "rs485-de";
>>    };
>> };
>>
>> &uart5 { /* DHCOM UART2 */
>>    pinctrl-0 = <&pinctrl_uart5>;
>>    pinctrl-1 = <&pinctrl_uart5_init>;
>>    pinctrl-names = "default", "init";
>>    ...
>> };
>> pinctrl_uart5_init: uart5-init-grp {
>>    fsl,pins = <
>> ...
>>      MX6QDL_PAD_CSI0_DAT19__GPIO6_IO05       0x30b1
>>    >;
>> };
>>
>> pinctrl_uart5: uart5-grp {
>>    fsl,pins = <
>> ...
>>      MX6QDL_PAD_CSI0_DAT19__UART5_CTS_B      0x30b1
>>    >;
>> };
>> "
> 
> An alternative work around is to use the GPIO as RTS also in default,
> isn't it?

No, see paragraph 6 in the wall of text above. The CTSC/CTS is 
synchronous to the internal UART IP clock. The GPIO workaround has 
downsides due to switching lag which breaks various deployments. Here 
the GPIO is only used to keep the CTS line LOW on boot ; after the 
driver takes over, the driver controls the CTS line all the time.

>> @@ -489,7 +489,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
>>   static void imx_uart_stop_rx(struct uart_port *port)
>>   {
>>   	struct imx_port *sport = (struct imx_port *)port;
>> -	u32 ucr1, ucr2, ucr4;
>> +	u32 ucr1, ucr2, ucr4, uts;
>>   
>>   	ucr1 = imx_uart_readl(sport, UCR1);
>>   	ucr2 = imx_uart_readl(sport, UCR2);
>> @@ -505,7 +505,17 @@ static void imx_uart_stop_rx(struct uart_port *port)
>>   	imx_uart_writel(sport, ucr1, UCR1);
>>   	imx_uart_writel(sport, ucr4, UCR4);
>>   
>> -	ucr2 &= ~UCR2_RXEN;
>> +	if (port->rs485.flags & SER_RS485_ENABLED &&
>> +	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
>> +	    sport->have_rtscts && !sport->have_rtsgpio) {
> 
> IMHO this needs a comment. The later hunks might benefit from a comment
> two. I'd explain it once in more detail and refer to that description
> from the other relevant code sections.

Yes, I can add some abbreviated version of the commit message as a comment.

Re: [PATCH] tty: serial: imx: Handle RS485 DE signal active high

[Marek Vasut]

On 9/28/22 09:43, Uwe Kleine-König wrote:
> On Wed, Sep 28, 2022 at 06:40:37AM +0200, Marek Vasut wrote:
>> The default polarity of RS485 DE signal is active high. This driver does
>> not handle such case properly. Currently, when a pin is multiplexed as a
>> UART CTS_B on boot, this pin is pulled HIGH by the i.MX UART CTS circuit,
>> which activates DE signal on the RS485 transceiver and thus behave as if
>> the RS485 was transmitting data, so the system blocks the RS485 bus when
>> it starts and until user application takes over. This behavior is not OK.
>> The problem consists of two separate parts.
>>
>> First, the i.MX UART IP requires UCR1 UARTEN and UCR2 RXEN to be set for
>> UCR2 CTSC and CTS bits to have any effect. The UCR2 CTSC bit permits the
>> driver to set CTS (RTS_B or RS485 DE signal) to either level sychronous
>> to the internal UART IP clock. Compared to other options, like GPIO CTS
>> control, this has the benefit of being synchronous to the UART IP clock
>> and thus without glitches or bus delays. The reason for the CTS design
>> is likely because when the Receiver is disabled, the UART IP can never
>> indicate that it is ready to receive data by assering CTS signal, so
>> the CTS is always pulled HIGH by default.
>>
>> When the port is closed by user space, imx_uart_stop_rx() clears UCR2
>> RXEN bit, and imx_uart_shutdown() clears UCR1 UARTEN bit. This disables
>> UART Receiver and UART itself, and forces CTS signal HIGH, which leads
>> to the RS485 bus being blocked because RS485 DE is incorrectly active.
>>
>> The proposed solution for this problem is to keep the Receiver running
>> even after the port is closed, but in loopback mode. This disconnects
>> the RX FIFO input from the RXD external signal, and since UCR2 TXEN is
>> cleared, the UART Transmitter is disabled, so nothing can feed data in
>> the RX FIFO. Because the Receiver is still enabled, the UCR2 CTSC and
>> CTS bits still have effect and the CTS (RS485 DE) control is retained.
>>
>> Note that in case of RS485 DE signal active low, there is no problem and
>> no special handling is necessary. The CTS signal defaults to HIGH, thus
>> the RS485 is by default set to Receive and the bus is not blocked.
>>
>> Note that while there is the possibility to control CTS using GPIO with
>> either CTS polarity, this has the downside of not being synchronous to
>> the UART IP clock and thus glitchy and susceptible to slow DE switching.
>>
>> Second, on boot, before the UART driver probe callback is called, the
>> driver core triggers pinctrl_init_done() and configures the IOMUXC to
>> default state. At this point, UCR1 UARTEN and UCR2 RXEN are both still
>> cleared, but UART CTS_B (RS485 DE) is configured as CTS function, thus
>> the RTS signal is pulled HIGH by the UART IP CTS circuit.
>>
>> One part of the solution here is to enable UCR1 UARTEN and UCR2 RXEN and
>> UTS loopback in this driver probe callback, thus unblocking the CTSC and
>> CTS control early on. But this is still too late, since the pin control
>> is already configured and CTS has been pulled HIGH for a short period
>> of time.
>>
>> When Linux kernel boots and this driver is bound, the pin control is set
>> to special "init" state if the state is available, and driver can switch
>> the "default" state afterward when ready. This state can be used to set
>> the CTS line as a GPIO in DT temporarily, and a GPIO hog can force such
>> GPIO to LOW, thus keeping the RS485 DE line LOW early on boot. Once the
>> driver takes over and UCR1 UARTEN and UCR2 RXEN and UTS loopback are all
>> enabled, the driver can switch to "default" pin control state and control
>> the CTS line as function instead. DT binding example is below:
>>
>> "
>> &gpio6 {
>>    rts-init-hog {
>>      gpio-hog;
>>      gpios = <5 0>;
>>      output-low;
>>      line-name = "rs485-de";
>>    };
>> };
>>
>> &uart5 { /* DHCOM UART2 */
>>    pinctrl-0 = <&pinctrl_uart5>;
>>    pinctrl-1 = <&pinctrl_uart5_init>;
>>    pinctrl-names = "default", "init";
>>    ...
>> };
>> pinctrl_uart5_init: uart5-init-grp {
>>    fsl,pins = <
>> ...
>>      MX6QDL_PAD_CSI0_DAT19__GPIO6_IO05       0x30b1
>>    >;
>> };
>>
>> pinctrl_uart5: uart5-grp {
>>    fsl,pins = <
>> ...
>>      MX6QDL_PAD_CSI0_DAT19__UART5_CTS_B      0x30b1
>>    >;
>> };
>> "
> 
> An alternative work around is to use the GPIO as RTS also in default,
> isn't it?

No, see paragraph 6 in the wall of text above. The CTSC/CTS is 
synchronous to the internal UART IP clock. The GPIO workaround has 
downsides due to switching lag which breaks various deployments. Here 
the GPIO is only used to keep the CTS line LOW on boot ; after the 
driver takes over, the driver controls the CTS line all the time.

>> @@ -489,7 +489,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
>>   static void imx_uart_stop_rx(struct uart_port *port)
>>   {
>>   	struct imx_port *sport = (struct imx_port *)port;
>> -	u32 ucr1, ucr2, ucr4;
>> +	u32 ucr1, ucr2, ucr4, uts;
>>   
>>   	ucr1 = imx_uart_readl(sport, UCR1);
>>   	ucr2 = imx_uart_readl(sport, UCR2);
>> @@ -505,7 +505,17 @@ static void imx_uart_stop_rx(struct uart_port *port)
>>   	imx_uart_writel(sport, ucr1, UCR1);
>>   	imx_uart_writel(sport, ucr4, UCR4);
>>   
>> -	ucr2 &= ~UCR2_RXEN;
>> +	if (port->rs485.flags & SER_RS485_ENABLED &&
>> +	    port->rs485.flags & SER_RS485_RTS_ON_SEND &&
>> +	    sport->have_rtscts && !sport->have_rtsgpio) {
> 
> IMHO this needs a comment. The later hunks might benefit from a comment
> two. I'd explain it once in more detail and refer to that description
> from the other relevant code sections.

Yes, I can add some abbreviated version of the commit message as a comment.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] tty: serial: imx: Handle RS485 DE signal active high

[Christoph Niedermaier]

From: Marek Vasut [mailto:marex@denx.de]
Sent: Wednesday, September 28, 2022 6:41 AM
> The default polarity of RS485 DE signal is active high. This driver does
> not handle such case properly. Currently, when a pin is multiplexed as a
> UART CTS_B on boot, this pin is pulled HIGH by the i.MX UART CTS circuit,
> which activates DE signal on the RS485 transceiver and thus behave as if
> the RS485 was transmitting data, so the system blocks the RS485 bus when
> it starts and until user application takes over. This behavior is not OK.
> The problem consists of two separate parts.
> 
> First, the i.MX UART IP requires UCR1 UARTEN and UCR2 RXEN to be set for
> UCR2 CTSC and CTS bits to have any effect. The UCR2 CTSC bit permits the
> driver to set CTS (RTS_B or RS485 DE signal) to either level sychronous
> to the internal UART IP clock. Compared to other options, like GPIO CTS
> control, this has the benefit of being synchronous to the UART IP clock
> and thus without glitches or bus delays. The reason for the CTS design
> is likely because when the Receiver is disabled, the UART IP can never
> indicate that it is ready to receive data by assering CTS signal, so
> the CTS is always pulled HIGH by default.
> 
> When the port is closed by user space, imx_uart_stop_rx() clears UCR2
> RXEN bit, and imx_uart_shutdown() clears UCR1 UARTEN bit. This disables
> UART Receiver and UART itself, and forces CTS signal HIGH, which leads
> to the RS485 bus being blocked because RS485 DE is incorrectly active.
> 
> The proposed solution for this problem is to keep the Receiver running
> even after the port is closed, but in loopback mode. This disconnects
> the RX FIFO input from the RXD external signal, and since UCR2 TXEN is
> cleared, the UART Transmitter is disabled, so nothing can feed data in
> the RX FIFO. Because the Receiver is still enabled, the UCR2 CTSC and
> CTS bits still have effect and the CTS (RS485 DE) control is retained.
> 
> Note that in case of RS485 DE signal active low, there is no problem and
> no special handling is necessary. The CTS signal defaults to HIGH, thus
> the RS485 is by default set to Receive and the bus is not blocked.
> 
> Note that while there is the possibility to control CTS using GPIO with
> either CTS polarity, this has the downside of not being synchronous to
> the UART IP clock and thus glitchy and susceptible to slow DE switching.
> 
> Second, on boot, before the UART driver probe callback is called, the
> driver core triggers pinctrl_init_done() and configures the IOMUXC to
> default state. At this point, UCR1 UARTEN and UCR2 RXEN are both still
> cleared, but UART CTS_B (RS485 DE) is configured as CTS function, thus
> the RTS signal is pulled HIGH by the UART IP CTS circuit.
> 
> One part of the solution here is to enable UCR1 UARTEN and UCR2 RXEN and
> UTS loopback in this driver probe callback, thus unblocking the CTSC and
> CTS control early on. But this is still too late, since the pin control
> is already configured and CTS has been pulled HIGH for a short period
> of time.
> 
> When Linux kernel boots and this driver is bound, the pin control is set
> to special "init" state if the state is available, and driver can switch
> the "default" state afterward when ready. This state can be used to set
> the CTS line as a GPIO in DT temporarily, and a GPIO hog can force such
> GPIO to LOW, thus keeping the RS485 DE line LOW early on boot. Once the
> driver takes over and UCR1 UARTEN and UCR2 RXEN and UTS loopback are all
> enabled, the driver can switch to "default" pin control state and control
> the CTS line as function instead. DT binding example is below:
> 
> "
> &gpio6 {
>   rts-init-hog {
>     gpio-hog;
>     gpios = <5 0>;
>     output-low;
>     line-name = "rs485-de";
>   };
> };
> 
> &uart5 { /* DHCOM UART2 */
>   pinctrl-0 = <&pinctrl_uart5>;
>   pinctrl-1 = <&pinctrl_uart5_init>;
>   pinctrl-names = "default", "init";
>   ...
> };
> pinctrl_uart5_init: uart5-init-grp {
>   fsl,pins = <
> ...
>     MX6QDL_PAD_CSI0_DAT19__GPIO6_IO05       0x30b1
>   >;
> };
> 
> pinctrl_uart5: uart5-grp {
>   fsl,pins = <
> ...
>     MX6QDL_PAD_CSI0_DAT19__UART5_CTS_B      0x30b1
>   >;
> };
> "
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Fabio Estevam <festevam@denx.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: kernel@dh-electronics.com
> Cc: kernel@pengutronix.de
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/imx.c | 51 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 05b432dc7a85c..144f1cedd4b64 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -489,7 +489,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
>  static void imx_uart_stop_rx(struct uart_port *port)
>  {
>         struct imx_port *sport = (struct imx_port *)port;
> -       u32 ucr1, ucr2, ucr4;
> +       u32 ucr1, ucr2, ucr4, uts;
> 
>         ucr1 = imx_uart_readl(sport, UCR1);
>         ucr2 = imx_uart_readl(sport, UCR2);
> @@ -505,7 +505,17 @@ static void imx_uart_stop_rx(struct uart_port *port)
>         imx_uart_writel(sport, ucr1, UCR1);
>         imx_uart_writel(sport, ucr4, UCR4);
> 
> -       ucr2 &= ~UCR2_RXEN;
> +       if (port->rs485.flags & SER_RS485_ENABLED &&
> +           port->rs485.flags & SER_RS485_RTS_ON_SEND &&
> +           sport->have_rtscts && !sport->have_rtsgpio) {
> +               uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +               uts |= UTS_LOOP;
> +               imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +               ucr2 |= UCR2_RXEN;
> +       } else {
> +               ucr2 &= ~UCR2_RXEN;
> +       }
> +
>         imx_uart_writel(sport, ucr2, UCR2);
>  }
> 
> @@ -1393,7 +1403,7 @@ static int imx_uart_startup(struct uart_port *port)
>         int retval, i;
>         unsigned long flags;
>         int dma_is_inited = 0;
> -       u32 ucr1, ucr2, ucr3, ucr4;
> +       u32 ucr1, ucr2, ucr3, ucr4, uts;
> 
>         retval = clk_prepare_enable(sport->clk_per);
>         if (retval)
> @@ -1498,6 +1508,10 @@ static int imx_uart_startup(struct uart_port *port)
>                 imx_uart_writel(sport, ucr2, UCR2);
>         }
> 
> +       uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +       uts &= ~UTS_LOOP;
> +       imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +
>         spin_unlock_irqrestore(&sport->port.lock, flags);
> 
>         return 0;
> @@ -1507,7 +1521,7 @@ static void imx_uart_shutdown(struct uart_port *port)
>  {
>         struct imx_port *sport = (struct imx_port *)port;
>         unsigned long flags;
> -       u32 ucr1, ucr2, ucr4;
> +       u32 ucr1, ucr2, ucr4, uts;
> 
>         if (sport->dma_is_enabled) {
>                 dmaengine_terminate_sync(sport->dma_chan_tx);
> @@ -1551,7 +1565,17 @@ static void imx_uart_shutdown(struct uart_port *port)
>         spin_lock_irqsave(&sport->port.lock, flags);
> 
>         ucr1 = imx_uart_readl(sport, UCR1);
> -       ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN |
> UCR1_ATDMAEN);
> +       ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
> +       if (port->rs485.flags & SER_RS485_ENABLED &&
> +           port->rs485.flags & SER_RS485_RTS_ON_SEND &&
> +           sport->have_rtscts && !sport->have_rtsgpio) {
> +               uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +               uts |= UTS_LOOP;
> +               imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +               ucr1 |= UCR1_UARTEN;
> +       } else {
> +               ucr1 &= ~UCR1_UARTEN;
> +       }
>         imx_uart_writel(sport, ucr1, UCR1);
> 
>         ucr4 = imx_uart_readl(sport, UCR4);
> @@ -2213,7 +2237,7 @@ static int imx_uart_probe(struct platform_device *pdev)
>         void __iomem *base;
>         u32 dma_buf_conf[2];
>         int ret = 0;
> -       u32 ucr1;
> +       u32 ucr1, ucr2, uts;
>         struct resource *res;
>         int txirq, rxirq, rtsirq;
> 
> @@ -2350,6 +2374,21 @@ static int imx_uart_probe(struct platform_device *pdev)
>         ucr1 &= ~(UCR1_ADEN | UCR1_TRDYEN | UCR1_IDEN | UCR1_RRDYEN | UCR1_RTSDEN);
>         imx_uart_writel(sport, ucr1, UCR1);
> 
> +       if (sport->port.rs485.flags & SER_RS485_ENABLED &&
> +           sport->have_rtscts && !sport->have_rtsgpio) {
> +               uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +               uts |= UTS_LOOP;
> +               imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +
> +               ucr1 = imx_uart_readl(sport, UCR1);
> +               ucr1 |= UCR1_UARTEN;
> +               imx_uart_writel(sport, ucr1, UCR1);
> +
> +               ucr2 = imx_uart_readl(sport, UCR2);
> +               ucr2 |= UCR2_RXEN;
> +               imx_uart_writel(sport, ucr2, UCR2);
> +       }
> +
>         if (!imx_uart_is_imx1(sport) && sport->dte_mode) {
>                 /*
>                  * The DCEDTE bit changes the direction of DSR, DCD, DTR and RI
> --
> 2.35.1

Tested-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>


Regards
Christoph
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] tty: serial: imx: Handle RS485 DE signal active high

[Christoph Niedermaier]

From: Marek Vasut [mailto:marex@denx.de]
Sent: Wednesday, September 28, 2022 6:41 AM
> The default polarity of RS485 DE signal is active high. This driver does
> not handle such case properly. Currently, when a pin is multiplexed as a
> UART CTS_B on boot, this pin is pulled HIGH by the i.MX UART CTS circuit,
> which activates DE signal on the RS485 transceiver and thus behave as if
> the RS485 was transmitting data, so the system blocks the RS485 bus when
> it starts and until user application takes over. This behavior is not OK.
> The problem consists of two separate parts.
> 
> First, the i.MX UART IP requires UCR1 UARTEN and UCR2 RXEN to be set for
> UCR2 CTSC and CTS bits to have any effect. The UCR2 CTSC bit permits the
> driver to set CTS (RTS_B or RS485 DE signal) to either level sychronous
> to the internal UART IP clock. Compared to other options, like GPIO CTS
> control, this has the benefit of being synchronous to the UART IP clock
> and thus without glitches or bus delays. The reason for the CTS design
> is likely because when the Receiver is disabled, the UART IP can never
> indicate that it is ready to receive data by assering CTS signal, so
> the CTS is always pulled HIGH by default.
> 
> When the port is closed by user space, imx_uart_stop_rx() clears UCR2
> RXEN bit, and imx_uart_shutdown() clears UCR1 UARTEN bit. This disables
> UART Receiver and UART itself, and forces CTS signal HIGH, which leads
> to the RS485 bus being blocked because RS485 DE is incorrectly active.
> 
> The proposed solution for this problem is to keep the Receiver running
> even after the port is closed, but in loopback mode. This disconnects
> the RX FIFO input from the RXD external signal, and since UCR2 TXEN is
> cleared, the UART Transmitter is disabled, so nothing can feed data in
> the RX FIFO. Because the Receiver is still enabled, the UCR2 CTSC and
> CTS bits still have effect and the CTS (RS485 DE) control is retained.
> 
> Note that in case of RS485 DE signal active low, there is no problem and
> no special handling is necessary. The CTS signal defaults to HIGH, thus
> the RS485 is by default set to Receive and the bus is not blocked.
> 
> Note that while there is the possibility to control CTS using GPIO with
> either CTS polarity, this has the downside of not being synchronous to
> the UART IP clock and thus glitchy and susceptible to slow DE switching.
> 
> Second, on boot, before the UART driver probe callback is called, the
> driver core triggers pinctrl_init_done() and configures the IOMUXC to
> default state. At this point, UCR1 UARTEN and UCR2 RXEN are both still
> cleared, but UART CTS_B (RS485 DE) is configured as CTS function, thus
> the RTS signal is pulled HIGH by the UART IP CTS circuit.
> 
> One part of the solution here is to enable UCR1 UARTEN and UCR2 RXEN and
> UTS loopback in this driver probe callback, thus unblocking the CTSC and
> CTS control early on. But this is still too late, since the pin control
> is already configured and CTS has been pulled HIGH for a short period
> of time.
> 
> When Linux kernel boots and this driver is bound, the pin control is set
> to special "init" state if the state is available, and driver can switch
> the "default" state afterward when ready. This state can be used to set
> the CTS line as a GPIO in DT temporarily, and a GPIO hog can force such
> GPIO to LOW, thus keeping the RS485 DE line LOW early on boot. Once the
> driver takes over and UCR1 UARTEN and UCR2 RXEN and UTS loopback are all
> enabled, the driver can switch to "default" pin control state and control
> the CTS line as function instead. DT binding example is below:
> 
> "
> &gpio6 {
>   rts-init-hog {
>     gpio-hog;
>     gpios = <5 0>;
>     output-low;
>     line-name = "rs485-de";
>   };
> };
> 
> &uart5 { /* DHCOM UART2 */
>   pinctrl-0 = <&pinctrl_uart5>;
>   pinctrl-1 = <&pinctrl_uart5_init>;
>   pinctrl-names = "default", "init";
>   ...
> };
> pinctrl_uart5_init: uart5-init-grp {
>   fsl,pins = <
> ...
>     MX6QDL_PAD_CSI0_DAT19__GPIO6_IO05       0x30b1
>   >;
> };
> 
> pinctrl_uart5: uart5-grp {
>   fsl,pins = <
> ...
>     MX6QDL_PAD_CSI0_DAT19__UART5_CTS_B      0x30b1
>   >;
> };
> "
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Fabio Estevam <festevam@denx.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jirislaby@kernel.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: kernel@dh-electronics.com
> Cc: kernel@pengutronix.de
> Cc: linux-arm-kernel@lists.infradead.org
> To: linux-serial@vger.kernel.org
> ---
>  drivers/tty/serial/imx.c | 51 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 05b432dc7a85c..144f1cedd4b64 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -489,7 +489,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
>  static void imx_uart_stop_rx(struct uart_port *port)
>  {
>         struct imx_port *sport = (struct imx_port *)port;
> -       u32 ucr1, ucr2, ucr4;
> +       u32 ucr1, ucr2, ucr4, uts;
> 
>         ucr1 = imx_uart_readl(sport, UCR1);
>         ucr2 = imx_uart_readl(sport, UCR2);
> @@ -505,7 +505,17 @@ static void imx_uart_stop_rx(struct uart_port *port)
>         imx_uart_writel(sport, ucr1, UCR1);
>         imx_uart_writel(sport, ucr4, UCR4);
> 
> -       ucr2 &= ~UCR2_RXEN;
> +       if (port->rs485.flags & SER_RS485_ENABLED &&
> +           port->rs485.flags & SER_RS485_RTS_ON_SEND &&
> +           sport->have_rtscts && !sport->have_rtsgpio) {
> +               uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +               uts |= UTS_LOOP;
> +               imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +               ucr2 |= UCR2_RXEN;
> +       } else {
> +               ucr2 &= ~UCR2_RXEN;
> +       }
> +
>         imx_uart_writel(sport, ucr2, UCR2);
>  }
> 
> @@ -1393,7 +1403,7 @@ static int imx_uart_startup(struct uart_port *port)
>         int retval, i;
>         unsigned long flags;
>         int dma_is_inited = 0;
> -       u32 ucr1, ucr2, ucr3, ucr4;
> +       u32 ucr1, ucr2, ucr3, ucr4, uts;
> 
>         retval = clk_prepare_enable(sport->clk_per);
>         if (retval)
> @@ -1498,6 +1508,10 @@ static int imx_uart_startup(struct uart_port *port)
>                 imx_uart_writel(sport, ucr2, UCR2);
>         }
> 
> +       uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +       uts &= ~UTS_LOOP;
> +       imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +
>         spin_unlock_irqrestore(&sport->port.lock, flags);
> 
>         return 0;
> @@ -1507,7 +1521,7 @@ static void imx_uart_shutdown(struct uart_port *port)
>  {
>         struct imx_port *sport = (struct imx_port *)port;
>         unsigned long flags;
> -       u32 ucr1, ucr2, ucr4;
> +       u32 ucr1, ucr2, ucr4, uts;
> 
>         if (sport->dma_is_enabled) {
>                 dmaengine_terminate_sync(sport->dma_chan_tx);
> @@ -1551,7 +1565,17 @@ static void imx_uart_shutdown(struct uart_port *port)
>         spin_lock_irqsave(&sport->port.lock, flags);
> 
>         ucr1 = imx_uart_readl(sport, UCR1);
> -       ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN |
> UCR1_ATDMAEN);
> +       ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
> +       if (port->rs485.flags & SER_RS485_ENABLED &&
> +           port->rs485.flags & SER_RS485_RTS_ON_SEND &&
> +           sport->have_rtscts && !sport->have_rtsgpio) {
> +               uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +               uts |= UTS_LOOP;
> +               imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +               ucr1 |= UCR1_UARTEN;
> +       } else {
> +               ucr1 &= ~UCR1_UARTEN;
> +       }
>         imx_uart_writel(sport, ucr1, UCR1);
> 
>         ucr4 = imx_uart_readl(sport, UCR4);
> @@ -2213,7 +2237,7 @@ static int imx_uart_probe(struct platform_device *pdev)
>         void __iomem *base;
>         u32 dma_buf_conf[2];
>         int ret = 0;
> -       u32 ucr1;
> +       u32 ucr1, ucr2, uts;
>         struct resource *res;
>         int txirq, rxirq, rtsirq;
> 
> @@ -2350,6 +2374,21 @@ static int imx_uart_probe(struct platform_device *pdev)
>         ucr1 &= ~(UCR1_ADEN | UCR1_TRDYEN | UCR1_IDEN | UCR1_RRDYEN | UCR1_RTSDEN);
>         imx_uart_writel(sport, ucr1, UCR1);
> 
> +       if (sport->port.rs485.flags & SER_RS485_ENABLED &&
> +           sport->have_rtscts && !sport->have_rtsgpio) {
> +               uts = imx_uart_readl(sport, imx_uart_uts_reg(sport));
> +               uts |= UTS_LOOP;
> +               imx_uart_writel(sport, uts, imx_uart_uts_reg(sport));
> +
> +               ucr1 = imx_uart_readl(sport, UCR1);
> +               ucr1 |= UCR1_UARTEN;
> +               imx_uart_writel(sport, ucr1, UCR1);
> +
> +               ucr2 = imx_uart_readl(sport, UCR2);
> +               ucr2 |= UCR2_RXEN;
> +               imx_uart_writel(sport, ucr2, UCR2);
> +       }
> +
>         if (!imx_uart_is_imx1(sport) && sport->dte_mode) {
>                 /*
>                  * The DCEDTE bit changes the direction of DSR, DCD, DTR and RI
> --
> 2.35.1

Tested-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>


Regards
Christoph