rs485 properties: platform agnosticism + RTS polarity

Rework the common helper function for retrieving rs485 properties to be platform agnostic (patch [1/5]) and to allow specifying the RTS polarity (patch [2/5] and [3/5]).

Amend the fsl_lpuart and imx drivers to take advantage of this and fix a bunch of bugs while at it (patch [4/5] and [5/5]).

Note that Michal Oleszczyk posted a patch yesterday which documents the "rs485-rts-active-high" property supported by omap-serial in a way which suggests that *all* drivers default to active low, which is not the case (all other drivers default to active high). I think the present series is a better approach, but let's have a technical discussion about that.

I posted an earlier version of patch [1/5] a while back but withdrew it because I wanted to change uart_get_rs485_mode() to take a struct dev and a struct serial_rs485, instead of a struct uart_port (which seemed too fragile on second thought). That change has been made now.

Thanks,

Lukas

Lukas Wunner (5):
serial: Make retrieval of rs485 properties platform-agnostic
dt-bindings: serial: Add common rs485 binding for RTS polarity
serial: core: Support common rs485 binding for RTS polarity
serial: fsl_lpuart: Support common rs485 binding for RTS polarity
serial: imx: Support common rs485 binding for RTS polarity

 .../devicetree/bindings/serial/fsl-imx-uart.txt    |  3 ++-
 .../devicetree/bindings/serial/fsl-lpuart.txt      |  3 ++-
 .../devicetree/bindings/serial/omap_serial.txt     |  1 +
 Documentation/devicetree/bindings/serial/rs485.txt |  1 +
 drivers/tty/serial/atmel_serial.c                  |  2 +-
 drivers/tty/serial/fsl_lpuart.c                    | 15 ++++----------
 drivers/tty/serial/imx.c                           | 11 ++++++++---
 drivers/tty/serial/omap-serial.c                   | 11 +++++++----
 drivers/tty/serial/serial_core.c                   | 23 +++++++++++++++-------
 include/linux/serial_core.h                        |  3 +--
 10 files changed, 43 insertions(+), 30 deletions(-)

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

rs485 allows for robust half-duplex serial communication. It is often implemented by attaching an rs485 transceiver to a UART. The UART's RTS line is wired to the transceiver's Transmit Enable pin and determines whether the transceiver is sending or receiving.

Examples for such transceivers are Maxim MAX13451E and TI SN65HVD1781A:
https://datasheets.maximintegrated.com/en/ds/MAX13450E-MAX13451E.pdf
http://www.ti.com/lit/ds/symlink/sn65hvd1781a-q1.pdf

In the devicetree, the transceiver itself is not represented, only the UART is. A few rs485-specific dt-bindings already exist and these go into the UART's device node.

This commit adds a binding to set the RTS polarity. Most (if not all) transceivers require the Transmit Enable pin be driven high for sending, but in some cases boards may negate the pin and RTS must then be driven low. Consequently the polarity defaults to active high but can be inverted with the newly added "rs485-rts-active-low" binding.

Document this binding in rs485.txt and in the two drivers fsl-imx-uart and fsl-lpuart that are about to be amended with support for it.

Curiously, the omap_serial driver defaults to active low and already supports an "rs485-rts-active-high" binding to invert the polarity. This is left unchanged to retain compatibility, but the binding is herewith documented.

---
 Documentation/devicetree/bindings/serial/fsl-imx-uart.txt | 3 ++-
 Documentation/devicetree/bindings/serial/fsl-lpuart.txt   | 3 ++-
 Documentation/devicetree/bindings/serial/omap_serial.txt  | 1 +
 Documentation/devicetree/bindings/serial/rs485.txt        | 1 +
 4 files changed, 6 insertions(+), 2 deletions(-)

index 860a9559839a..89b92c1314cf 100644
@@ -9,7 +9,8 @@ Optional properties:
 - fsl,irda-mode : Indicate the uart supports irda mode
 - fsl,dte-mode : Indicate the uart works in DTE mode. The uart works
                   in DCE mode by default.
-- rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt
+- rs485-rts-delay, rs485-rts-active-low, rs485-rx-during-tx,
+  linux,rs485-enabled-at-boot-time: see rs485.txt
 
 Please check Documentation/devicetree/bindings/serial/serial.txt
 for the complete list of generic properties.

index 59567b51cf09..6bd3f2e93d61 100644
@@ -16,7 +16,8 @@ Required properties:
 Optional properties:
 - dmas: A list of two dma specifiers, one for each entry in dma-names.
 - dma-names: should contain "tx" and "rx".
-- rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt
+- rs485-rts-delay, rs485-rts-active-low, rs485-rx-during-tx,
+  linux,rs485-enabled-at-boot-time: see rs485.txt
 
 Note: Optional properties for DMA support. Write them both or both not.
 

index 43eac675f21f..4b0f05adb228 100644
@@ -20,6 +20,7 @@ Optional properties:
          node and a DMA channel number.
 - dma-names : "rx" for receive channel, "tx" for transmit channel.
 - rs485-rts-delay, rs485-rx-during-tx, linux,rs485-enabled-at-boot-time: see rs485.txt
+- rs485-rts-active-high: drive RTS high when sending (default is low).
 
 Example:
 

index b8415936dfdb..b7c29f74ebb2 100644
@@ -12,6 +12,7 @@ Optional properties:
   * b is the delay between end of data sent and rts signal in milliseconds
       it corresponds to the delay after sending data and actual release of the line.
   If this property is not specified, <0 0> is assumed.
+- rs485-rts-active-low: drive RTS low when sending (default is high).
 - linux,rs485-enabled-at-boot-time: empty property telling to enable the rs485
   feature at boot time. It can be disabled later with proper ioctl.
 - rs485-rx-during-tx: empty property that enables the receiving of data even

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

That often ends up being a mistake. We don't describe things, then add a few properties and then finally realize we need a full representation. Connectors are a good example.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

When a driver invokes the uart_get_rs485_mode() helper, set the RTS polarity to active high by default unless the newly introduced "rs485-rts-active-low" property was specified.

omap-serial historically defaults to active low and supports an "rs485-rts-active-high" property to inverse the polarity. Retain that behavior for compatibility.

---
 drivers/tty/serial/omap-serial.c | 7 +++++--
 drivers/tty/serial/serial_core.c | 8 ++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

index e6aadb6d02e5..7ef36c0af825 100644
@@ -1617,10 +1617,13 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
 
 	uart_get_rs485_mode(up->dev, rs485conf);
 
-	if (of_property_read_bool(np, "rs485-rts-active-high"))
+	if (of_property_read_bool(np, "rs485-rts-active-high")) {
 		rs485conf->flags |= SER_RS485_RTS_ON_SEND;
-	else
+		rs485conf->flags &= ~SER_RS485_RTS_AFTER_SEND;
+	} else {
+		rs485conf->flags &= ~SER_RS485_RTS_ON_SEND;
 		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
+	}
 
 	/* check for tx enable gpio */
 	up->rts_gpio = of_get_named_gpio_flags(np, "rts-gpio", 0, &flags);

index 548bb80223c1..64e15507a9bd 100644
@@ -3048,6 +3048,14 @@ void uart_get_rs485_mode(struct device *dev, struct serial_rs485 *rs485conf)
 		rs485conf->delay_rts_after_send = 0;
 	}
 
+	if (device_property_read_bool(dev, "rs485-rts-active-low")) {
+		rs485conf->flags &= ~SER_RS485_RTS_ON_SEND;
+		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
+	} else {
+		rs485conf->flags |= SER_RS485_RTS_ON_SEND;
+		rs485conf->flags &= ~SER_RS485_RTS_AFTER_SEND;
+	}
+
 	/*
 	 * clear full-duplex and enabled flags to get to a defined state with
 	 * the two following properties.

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks a lot Uwe for your review comments, those are good points, d'accord on all of them. I've already incorporated all suggested changes into my local repo but will wait a few days before respinning to see if there are further comments.

Best,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Invoke the ->rs485_config callback on probe to set UARTMODEM_TXRTSPOL appropriately based on the UART's device properties.

This implicitly sets UARTMODEM_TXRTSE if rs485 was enabled in the device properties, so drop the identical code from lpuart_probe().

It also fixes a bug: If an unsupported rs485 property was specified (rs485-rx-during-tx or rs485-rts-delay), the driver returns -ENOSYS without performing any cleanup, in particular without calling uart_remove_one_port() or clk_disable_unprepare(), thus leaking the uart_port. But with the invocation of ->rs485_config, the unsupported properties are now cleared in struct serial_rs485 and thus ignored. It therefore seems sufficient to just log an error instead of bailing out.

---
 drivers/tty/serial/fsl_lpuart.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

index 009eb3f18a81..fbb8c3ce470b 100644
@@ -2213,21 +2213,14 @@ static int lpuart_probe(struct platform_device *pdev)
 
 	uart_get_rs485_mode(&pdev->dev, &sport->port.rs485);
 
-	if (sport->port.rs485.flags & SER_RS485_RX_DURING_TX) {
+	if (sport->port.rs485.flags & SER_RS485_RX_DURING_TX)
 		dev_err(&pdev->dev, "driver doesn't support RX during TX\n");
-		return -ENOSYS;
-	}
 
 	if (sport->port.rs485.delay_rts_before_send ||
-	    sport->port.rs485.delay_rts_after_send) {
+	    sport->port.rs485.delay_rts_after_send)
 		dev_err(&pdev->dev, "driver doesn't support RTS delays\n");
-		return -ENOSYS;
-	}
 
-	if (sport->port.rs485.flags & SER_RS485_ENABLED) {
-		sport->port.rs485.flags |= SER_RS485_RTS_ON_SEND;
-		writeb(UARTMODEM_TXRTSE, sport->port.membase + UARTMODEM);
-	}
+	lpuart_config_rs485(&sport->port, &sport->port.rs485);
 
 	sport->dma_tx_chan = dma_request_slave_channel(sport->port.dev, "tx");
 	if (!sport->dma_tx_chan)

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Invoke the ->rs485_config callback on probe to adjust the initial RTS polarity based on the UART's device properties.

This implicitly fixes a bug: If RTS control is not available, rs485 should be disabled even if it was enabled through a device property. Log an error when that occurs.

---
 drivers/tty/serial/imx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index 52456fb2bff9..50d4fdcd1aec 100644
@@ -2087,7 +2087,6 @@ static int serial_imx_probe(struct platform_device *pdev)
 	sport->port.fifosize = 32;
 	sport->port.ops = &imx_pops;
 	sport->port.rs485_config = imx_rs485_config;
-	sport->port.rs485.flags |= SER_RS485_RTS_ON_SEND;
 	sport->port.flags = UPF_BOOT_AUTOCONF;
 	setup_timer(&sport->timer, imx_timeout, (unsigned long)sport);
 

@@ -2120,6 +2119,12 @@ static int serial_imx_probe(struct platform_device *pdev)
 
 	uart_get_rs485_mode(&pdev->dev, &sport->port.rs485);
 
+	if (sport->port.rs485.flags & SER_RS485_ENABLED &&
+	    (!sport->have_rtscts || !sport->have_rtsgpio))
+		dev_err(&pdev->dev, "no RTS control, disabling rs485\n");
+
+	imx_rs485_config(&sport->port, &sport->port.rs485);
+
 	/* Disable interrupts before requesting them */
 	reg = readl_relaxed(sport->port.membase + UCR1);
 	reg &= ~(UCR1_ADEN | UCR1_TRDYEN | UCR1_IDEN | UCR1_RRDYEN |

2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html