[PATCH V3 0/2] Add rs485 support to uartps driver

[PATCH V3 0/2] Add rs485 support to uartps driver

[Manikanta Guntupalli]

Add optional gpio property to uartps node to support rs485.
Add rs485 support to uartps driver.
---
Changes for V2:
Modify optional gpio name to xlnx,phy-ctrl-gpios.
Update commit description.
Add support for RTS, delay_rts_before_send and delay_rts_after_send in RS485 mode.
Changes for V3:
Modify optional gpio name to rts-gpios.
Update commit description.
Move cdns_uart_tx_empty function to avoid prototype statement.
Remove assignment of struct serial_rs485 to port->rs485 as
serial core performs that.
Switch to native RTS in non GPIO case.
Handle rs485 during stop tx.
Remove explicit calls to configure gpio direction and value,
as devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW argument.
Update implementation to support configuration of GPIO/RTS value
based on user configuration of SER_RS485_RTS_ON_SEND and
SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from handle_tx.

Manikanta Guntupalli (2):
  dt-bindings: Add optional gpio property to uartps node to support
    rs485
  tty: serial: uartps: Add rs485 support to uartps driver

 .../devicetree/bindings/serial/cdns,uart.yaml |   6 +
 drivers/tty/serial/xilinx_uartps.c            | 180 ++++++++++++++++--
 2 files changed, 171 insertions(+), 15 deletions(-)

-- 
2.25.1

[PATCH V3 0/2] Add rs485 support to uartps driver

[Manikanta Guntupalli]

Add optional gpio property to uartps node to support rs485.
Add rs485 support to uartps driver.
---
Changes for V2:
Modify optional gpio name to xlnx,phy-ctrl-gpios.
Update commit description.
Add support for RTS, delay_rts_before_send and delay_rts_after_send in RS485 mode.
Changes for V3:
Modify optional gpio name to rts-gpios.
Update commit description.
Move cdns_uart_tx_empty function to avoid prototype statement.
Remove assignment of struct serial_rs485 to port->rs485 as
serial core performs that.
Switch to native RTS in non GPIO case.
Handle rs485 during stop tx.
Remove explicit calls to configure gpio direction and value,
as devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW argument.
Update implementation to support configuration of GPIO/RTS value
based on user configuration of SER_RS485_RTS_ON_SEND and
SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from handle_tx.

Manikanta Guntupalli (2):
  dt-bindings: Add optional gpio property to uartps node to support
    rs485
  tty: serial: uartps: Add rs485 support to uartps driver

 .../devicetree/bindings/serial/cdns,uart.yaml |   6 +
 drivers/tty/serial/xilinx_uartps.c            | 180 ++++++++++++++++--
 2 files changed, 171 insertions(+), 15 deletions(-)

-- 
2.25.1


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

[PATCH V3 1/2] dt-bindings: Add optional gpio property to uartps node to support rs485

[Manikanta Guntupalli]

Add optional gpio property to uartps node and reference to rs485.yaml

On Xilinx/AMD Kria SOM KD240 board rs485 connects via TI ISOW1432
Transceiver device, where one GPIO is used for driving DE/RE signals.
With rs485 half duplex configuration, DE and RE are shorted to each other,
and at a time, any node acts as either a driver or a receiver.

Here,
DE - Driver enable. If pin is floating, driver is disabled.
RE - Receiver enable. If pin is floating, receiver buffer is disabled.

For more deatils, please find below link which contains Transceiver
device(ISOW1432) datasheet
https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN

rts-gpios is optional property, because it is not required
for uart console node.

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
Changes for V2:
Modify optional gpio name to xlnx,phy-ctrl-gpios.
Update commit description.
Changes for V3:
Modify optional gpio name to rts-gpios.
Update commit description.
---
 Documentation/devicetree/bindings/serial/cdns,uart.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
index e35ad1109efc..7ee305f9a45f 100644
--- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
+++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
@@ -46,6 +46,11 @@ properties:
   power-domains:
     maxItems: 1
 
+  rts-gpios:
+    description: Optional GPIO to control transmit/receive on RS485 phy
+      in halfduplex mode.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -55,6 +60,7 @@ required:
 
 allOf:
   - $ref: serial.yaml#
+  - $ref: rs485.yaml#
   - if:
       properties:
         compatible:
-- 
2.25.1

[PATCH V3 1/2] dt-bindings: Add optional gpio property to uartps node to support rs485

[Manikanta Guntupalli]

Add optional gpio property to uartps node and reference to rs485.yaml

On Xilinx/AMD Kria SOM KD240 board rs485 connects via TI ISOW1432
Transceiver device, where one GPIO is used for driving DE/RE signals.
With rs485 half duplex configuration, DE and RE are shorted to each other,
and at a time, any node acts as either a driver or a receiver.

Here,
DE - Driver enable. If pin is floating, driver is disabled.
RE - Receiver enable. If pin is floating, receiver buffer is disabled.

For more deatils, please find below link which contains Transceiver
device(ISOW1432) datasheet
https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN

rts-gpios is optional property, because it is not required
for uart console node.

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
Changes for V2:
Modify optional gpio name to xlnx,phy-ctrl-gpios.
Update commit description.
Changes for V3:
Modify optional gpio name to rts-gpios.
Update commit description.
---
 Documentation/devicetree/bindings/serial/cdns,uart.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
index e35ad1109efc..7ee305f9a45f 100644
--- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
+++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
@@ -46,6 +46,11 @@ properties:
   power-domains:
     maxItems: 1
 
+  rts-gpios:
+    description: Optional GPIO to control transmit/receive on RS485 phy
+      in halfduplex mode.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -55,6 +60,7 @@ required:
 
 allOf:
   - $ref: serial.yaml#
+  - $ref: rs485.yaml#
   - if:
       properties:
         compatible:
-- 
2.25.1


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

[PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Manikanta Guntupalli]

Add rs485 support to uartps driver. Use either rts-gpios or RTS
to control RS485 phy as driver or a receiver.

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
Changes for V2:
Modify optional gpio name to xlnx,phy-ctrl-gpios.
Update commit description.
Add support for RTS, delay_rts_before_send and delay_rts_after_send in RS485 mode.
Changes for V3:
Modify optional gpio name to rts-gpios.
Update commit description.
Move cdns_uart_tx_empty function to avoid prototype statement.
Remove assignment of struct serial_rs485 to port->rs485 as
serial core performs that.
Switch to native RTS in non GPIO case.
Handle rs485 during stop tx.
Remove explicit calls to configure gpio direction and value,
as devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW argument.
Update implementation to support configuration of GPIO/RTS value
based on user configuration of SER_RS485_RTS_ON_SEND and
SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from handle_tx.
---
 drivers/tty/serial/xilinx_uartps.c | 180 ++++++++++++++++++++++++++---
 1 file changed, 165 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 9c13dac1d4d1..32229cf5c508 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -23,6 +23,9 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/iopoll.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
 
 #define CDNS_UART_TTY_NAME	"ttyPS"
 #define CDNS_UART_NAME		"xuartps"
@@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
  * @clk_rate_change_nb:	Notifier block for clock changes
  * @quirks:		Flags for RXBS support.
  * @cts_override:	Modem control state override
+ * @gpiod:		Pointer to the gpio descriptor
  */
 struct cdns_uart {
 	struct uart_port	*port;
@@ -203,10 +207,19 @@ struct cdns_uart {
 	struct notifier_block	clk_rate_change_nb;
 	u32			quirks;
 	bool cts_override;
+	struct gpio_desc	*gpiod;
 };
 struct cdns_platform_data {
 	u32 quirks;
 };
+
+struct serial_rs485 cdns_rs485_supported = {
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+		 SER_RS485_RTS_AFTER_SEND,
+	.delay_rts_before_send = 1,
+	.delay_rts_after_send = 1,
+};
+
 #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
 		clk_rate_change_nb)
 
@@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
 	tty_flip_buffer_push(&port->state->port);
 }
 
+/**
+ * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
+ * @cdns_uart: Handle to the cdns_uart
+ */
+static void cdns_rs485_config_gpio_rts_high(struct cdns_uart *cdns_uart)
+{
+	u32 val;
+
+	if (cdns_uart->gpiod) {
+		gpiod_set_value(cdns_uart->gpiod, 1);
+	} else {
+		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
+		val &= ~CDNS_UART_MODEMCR_RTS;
+		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
+	}
+}
+
+/**
+ * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
+ * @cdns_uart: Handle to the cdns_uart
+ */
+static void cdns_rs485_config_gpio_rts_low(struct cdns_uart *cdns_uart)
+{
+	u32 val;
+
+	if (cdns_uart->gpiod) {
+		gpiod_set_value(cdns_uart->gpiod, 0);
+	} else {
+		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
+		val |= CDNS_UART_MODEMCR_RTS;
+		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
+	}
+}
+
+/**
+ * cdns_rs485_tx_setup - Tx setup specific to rs485
+ * @cdns_uart: Handle to the cdns_uart
+ */
+static void cdns_rs485_tx_setup(struct cdns_uart *cdns_uart)
+{
+	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
+		cdns_rs485_config_gpio_rts_high(cdns_uart);
+	else
+		cdns_rs485_config_gpio_rts_low(cdns_uart);
+}
+
+/**
+ * cdns_rs485_rx_setup - Rx setup specific to rs485
+ * @cdns_uart: Handle to the cdns_uart
+ */
+static void cdns_rs485_rx_setup(struct cdns_uart *cdns_uart)
+{
+	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+		cdns_rs485_config_gpio_rts_high(cdns_uart);
+	else
+		cdns_rs485_config_gpio_rts_low(cdns_uart);
+}
+
+/**
+ * cdns_uart_tx_empty -  Check whether TX is empty
+ * @port: Handle to the uart port structure
+ *
+ * Return: TIOCSER_TEMT on success, 0 otherwise
+ */
+static unsigned int cdns_uart_tx_empty(struct uart_port *port)
+{
+	unsigned int status;
+
+	status = readl(port->membase + CDNS_UART_SR) &
+		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
+	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
+}
+
 /**
  * cdns_uart_handle_tx - Handle the bytes to be Txed.
  * @dev_id: Id of the UART port
@@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
 static void cdns_uart_start_tx(struct uart_port *port)
 {
 	unsigned int status;
+	unsigned long time_out;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	if (uart_tx_stopped(port))
 		return;
@@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port *port)
 
 	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
 
+	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
+		cdns_rs485_tx_setup(cdns_uart);
+		if (cdns_uart->port->rs485.delay_rts_before_send)
+			mdelay(cdns_uart->port->rs485.delay_rts_before_send);
+	}
+
 	cdns_uart_handle_tx(port);
 
+	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
+		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
+		/* Wait for tx completion */
+		while ((cdns_uart_tx_empty(cdns_uart->port) != TIOCSER_TEMT) &&
+		       time_before(jiffies, time_out))
+			cpu_relax();
+
+		if (cdns_uart->port->rs485.delay_rts_after_send)
+			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
+
+		/*
+		 * Default Rx should be setup, because RX signaling path
+		 * need to enable to receive data.
+		 */
+		cdns_rs485_rx_setup(cdns_uart);
+	}
+
 	/* Enable the TX Empty interrupt */
 	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
 }
@@ -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
 static void cdns_uart_stop_tx(struct uart_port *port)
 {
 	unsigned int regval;
+	struct cdns_uart *cdns_uart = port->private_data;
+
+	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
+		if (cdns_uart->port->rs485.delay_rts_after_send)
+			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
+
+		cdns_rs485_rx_setup(cdns_uart);
+	}
 
 	regval = readl(port->membase + CDNS_UART_CR);
 	regval |= CDNS_UART_CR_TX_DIS;
@@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port *port)
 	writel(regval, port->membase + CDNS_UART_CR);
 }
 
-/**
- * cdns_uart_tx_empty -  Check whether TX is empty
- * @port: Handle to the uart port structure
- *
- * Return: TIOCSER_TEMT on success, 0 otherwise
- */
-static unsigned int cdns_uart_tx_empty(struct uart_port *port)
-{
-	unsigned int status;
-
-	status = readl(port->membase + CDNS_UART_SR) &
-		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
-	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
-}
-
 /**
  * cdns_uart_break_ctl - Based on the input ctl we have to start or stop
  *			transmitting char breaks
@@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port *port)
 		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
 		cpu_relax();
 
+	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
+		cdns_rs485_rx_setup(cdns_uart);
+
 	/*
 	 * Clear the RX disable bit and then set the RX enable bit to enable
 	 * the receiver.
@@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
 /* Temporary variable for storing number of instances */
 static int instances;
 
+/**
+ * cdns_rs485_config - Called when an application calls TIOCSRS485 ioctl.
+ * @port: Pointer to the uart_port structure
+ * @termios: Pointer to the ktermios structure
+ * @rs485: Pointer to the serial_rs485 structure
+ *
+ * Return: 0
+ */
+static int cdns_rs485_config(struct uart_port *port, struct ktermios *termios,
+			     struct serial_rs485 *rs485)
+{
+	if (rs485->flags & SER_RS485_ENABLED)
+		dev_dbg(port->dev, "Setting UART to RS485\n");
+
+	return 0;
+}
+
 /**
  * cdns_uart_probe - Platform driver probe
  * @pdev: Pointer to the platform device structure
@@ -1463,6 +1587,7 @@ static int instances;
  */
 static int cdns_uart_probe(struct platform_device *pdev)
 {
+	u32 val;
 	int rc, id, irq;
 	struct uart_port *port;
 	struct resource *res;
@@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	port->private_data = cdns_uart_data;
 	port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG |
 			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
+	port->rs485_config = cdns_rs485_config;
+	port->rs485_supported = cdns_rs485_supported;
 	cdns_uart_data->port = port;
 	platform_set_drvdata(pdev, port);
 
+	rc = uart_get_rs485_mode(port);
+	if (rc)
+		goto err_out_clk_notifier;
+
+	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev, "rts",
+							GPIOD_OUT_LOW);
+	if (IS_ERR(cdns_uart_data->gpiod)) {
+		rc = PTR_ERR(cdns_uart_data->gpiod);
+		dev_err(port->dev, "xuartps: devm_gpiod_get_optional failed\n");
+		goto err_out_clk_notifier;
+	}
+
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
 	pm_runtime_set_active(&pdev->dev);
@@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
 							     "cts-override");
 
+	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
+		if (!cdns_uart_data->gpiod) {
+			val = readl(cdns_uart_data->port->membase
+				    + CDNS_UART_MODEMCR);
+			val |= CDNS_UART_MODEMCR_RTS;
+			writel(val, cdns_uart_data->port->membase
+			       + CDNS_UART_MODEMCR);
+		}
+	}
+
 	instances++;
 
 	return 0;
@@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
+err_out_clk_notifier:
 #ifdef CONFIG_COMMON_CLK
 	clk_notifier_unregister(cdns_uart_data->uartclk,
 			&cdns_uart_data->clk_rate_change_nb);
-- 
2.25.1

[PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Manikanta Guntupalli]

Add rs485 support to uartps driver. Use either rts-gpios or RTS
to control RS485 phy as driver or a receiver.

Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
---
Changes for V2:
Modify optional gpio name to xlnx,phy-ctrl-gpios.
Update commit description.
Add support for RTS, delay_rts_before_send and delay_rts_after_send in RS485 mode.
Changes for V3:
Modify optional gpio name to rts-gpios.
Update commit description.
Move cdns_uart_tx_empty function to avoid prototype statement.
Remove assignment of struct serial_rs485 to port->rs485 as
serial core performs that.
Switch to native RTS in non GPIO case.
Handle rs485 during stop tx.
Remove explicit calls to configure gpio direction and value,
as devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW argument.
Update implementation to support configuration of GPIO/RTS value
based on user configuration of SER_RS485_RTS_ON_SEND and
SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from handle_tx.
---
 drivers/tty/serial/xilinx_uartps.c | 180 ++++++++++++++++++++++++++---
 1 file changed, 165 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 9c13dac1d4d1..32229cf5c508 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -23,6 +23,9 @@
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/iopoll.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/delay.h>
 
 #define CDNS_UART_TTY_NAME	"ttyPS"
 #define CDNS_UART_NAME		"xuartps"
@@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
  * @clk_rate_change_nb:	Notifier block for clock changes
  * @quirks:		Flags for RXBS support.
  * @cts_override:	Modem control state override
+ * @gpiod:		Pointer to the gpio descriptor
  */
 struct cdns_uart {
 	struct uart_port	*port;
@@ -203,10 +207,19 @@ struct cdns_uart {
 	struct notifier_block	clk_rate_change_nb;
 	u32			quirks;
 	bool cts_override;
+	struct gpio_desc	*gpiod;
 };
 struct cdns_platform_data {
 	u32 quirks;
 };
+
+struct serial_rs485 cdns_rs485_supported = {
+	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+		 SER_RS485_RTS_AFTER_SEND,
+	.delay_rts_before_send = 1,
+	.delay_rts_after_send = 1,
+};
+
 #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
 		clk_rate_change_nb)
 
@@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
 	tty_flip_buffer_push(&port->state->port);
 }
 
+/**
+ * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
+ * @cdns_uart: Handle to the cdns_uart
+ */
+static void cdns_rs485_config_gpio_rts_high(struct cdns_uart *cdns_uart)
+{
+	u32 val;
+
+	if (cdns_uart->gpiod) {
+		gpiod_set_value(cdns_uart->gpiod, 1);
+	} else {
+		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
+		val &= ~CDNS_UART_MODEMCR_RTS;
+		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
+	}
+}
+
+/**
+ * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
+ * @cdns_uart: Handle to the cdns_uart
+ */
+static void cdns_rs485_config_gpio_rts_low(struct cdns_uart *cdns_uart)
+{
+	u32 val;
+
+	if (cdns_uart->gpiod) {
+		gpiod_set_value(cdns_uart->gpiod, 0);
+	} else {
+		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
+		val |= CDNS_UART_MODEMCR_RTS;
+		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
+	}
+}
+
+/**
+ * cdns_rs485_tx_setup - Tx setup specific to rs485
+ * @cdns_uart: Handle to the cdns_uart
+ */
+static void cdns_rs485_tx_setup(struct cdns_uart *cdns_uart)
+{
+	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
+		cdns_rs485_config_gpio_rts_high(cdns_uart);
+	else
+		cdns_rs485_config_gpio_rts_low(cdns_uart);
+}
+
+/**
+ * cdns_rs485_rx_setup - Rx setup specific to rs485
+ * @cdns_uart: Handle to the cdns_uart
+ */
+static void cdns_rs485_rx_setup(struct cdns_uart *cdns_uart)
+{
+	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+		cdns_rs485_config_gpio_rts_high(cdns_uart);
+	else
+		cdns_rs485_config_gpio_rts_low(cdns_uart);
+}
+
+/**
+ * cdns_uart_tx_empty -  Check whether TX is empty
+ * @port: Handle to the uart port structure
+ *
+ * Return: TIOCSER_TEMT on success, 0 otherwise
+ */
+static unsigned int cdns_uart_tx_empty(struct uart_port *port)
+{
+	unsigned int status;
+
+	status = readl(port->membase + CDNS_UART_SR) &
+		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
+	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
+}
+
 /**
  * cdns_uart_handle_tx - Handle the bytes to be Txed.
  * @dev_id: Id of the UART port
@@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
 static void cdns_uart_start_tx(struct uart_port *port)
 {
 	unsigned int status;
+	unsigned long time_out;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	if (uart_tx_stopped(port))
 		return;
@@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port *port)
 
 	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
 
+	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
+		cdns_rs485_tx_setup(cdns_uart);
+		if (cdns_uart->port->rs485.delay_rts_before_send)
+			mdelay(cdns_uart->port->rs485.delay_rts_before_send);
+	}
+
 	cdns_uart_handle_tx(port);
 
+	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
+		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
+		/* Wait for tx completion */
+		while ((cdns_uart_tx_empty(cdns_uart->port) != TIOCSER_TEMT) &&
+		       time_before(jiffies, time_out))
+			cpu_relax();
+
+		if (cdns_uart->port->rs485.delay_rts_after_send)
+			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
+
+		/*
+		 * Default Rx should be setup, because RX signaling path
+		 * need to enable to receive data.
+		 */
+		cdns_rs485_rx_setup(cdns_uart);
+	}
+
 	/* Enable the TX Empty interrupt */
 	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
 }
@@ -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
 static void cdns_uart_stop_tx(struct uart_port *port)
 {
 	unsigned int regval;
+	struct cdns_uart *cdns_uart = port->private_data;
+
+	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
+		if (cdns_uart->port->rs485.delay_rts_after_send)
+			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
+
+		cdns_rs485_rx_setup(cdns_uart);
+	}
 
 	regval = readl(port->membase + CDNS_UART_CR);
 	regval |= CDNS_UART_CR_TX_DIS;
@@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port *port)
 	writel(regval, port->membase + CDNS_UART_CR);
 }
 
-/**
- * cdns_uart_tx_empty -  Check whether TX is empty
- * @port: Handle to the uart port structure
- *
- * Return: TIOCSER_TEMT on success, 0 otherwise
- */
-static unsigned int cdns_uart_tx_empty(struct uart_port *port)
-{
-	unsigned int status;
-
-	status = readl(port->membase + CDNS_UART_SR) &
-		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
-	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
-}
-
 /**
  * cdns_uart_break_ctl - Based on the input ctl we have to start or stop
  *			transmitting char breaks
@@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port *port)
 		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
 		cpu_relax();
 
+	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
+		cdns_rs485_rx_setup(cdns_uart);
+
 	/*
 	 * Clear the RX disable bit and then set the RX enable bit to enable
 	 * the receiver.
@@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
 /* Temporary variable for storing number of instances */
 static int instances;
 
+/**
+ * cdns_rs485_config - Called when an application calls TIOCSRS485 ioctl.
+ * @port: Pointer to the uart_port structure
+ * @termios: Pointer to the ktermios structure
+ * @rs485: Pointer to the serial_rs485 structure
+ *
+ * Return: 0
+ */
+static int cdns_rs485_config(struct uart_port *port, struct ktermios *termios,
+			     struct serial_rs485 *rs485)
+{
+	if (rs485->flags & SER_RS485_ENABLED)
+		dev_dbg(port->dev, "Setting UART to RS485\n");
+
+	return 0;
+}
+
 /**
  * cdns_uart_probe - Platform driver probe
  * @pdev: Pointer to the platform device structure
@@ -1463,6 +1587,7 @@ static int instances;
  */
 static int cdns_uart_probe(struct platform_device *pdev)
 {
+	u32 val;
 	int rc, id, irq;
 	struct uart_port *port;
 	struct resource *res;
@@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	port->private_data = cdns_uart_data;
 	port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG |
 			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
+	port->rs485_config = cdns_rs485_config;
+	port->rs485_supported = cdns_rs485_supported;
 	cdns_uart_data->port = port;
 	platform_set_drvdata(pdev, port);
 
+	rc = uart_get_rs485_mode(port);
+	if (rc)
+		goto err_out_clk_notifier;
+
+	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev, "rts",
+							GPIOD_OUT_LOW);
+	if (IS_ERR(cdns_uart_data->gpiod)) {
+		rc = PTR_ERR(cdns_uart_data->gpiod);
+		dev_err(port->dev, "xuartps: devm_gpiod_get_optional failed\n");
+		goto err_out_clk_notifier;
+	}
+
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
 	pm_runtime_set_active(&pdev->dev);
@@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
 							     "cts-override");
 
+	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
+		if (!cdns_uart_data->gpiod) {
+			val = readl(cdns_uart_data->port->membase
+				    + CDNS_UART_MODEMCR);
+			val |= CDNS_UART_MODEMCR_RTS;
+			writel(val, cdns_uart_data->port->membase
+			       + CDNS_UART_MODEMCR);
+		}
+	}
+
 	instances++;
 
 	return 0;
@@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
+err_out_clk_notifier:
 #ifdef CONFIG_COMMON_CLK
 	clk_notifier_unregister(cdns_uart_data->uartclk,
 			&cdns_uart_data->clk_rate_change_nb);
-- 
2.25.1


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

Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Ilpo Järvinen]

On Tue, 24 Oct 2023, Manikanta Guntupalli wrote:

> Add rs485 support to uartps driver. Use either rts-gpios or RTS
> to control RS485 phy as driver or a receiver.
> 
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Modify optional gpio name to xlnx,phy-ctrl-gpios.
> Update commit description.
> Add support for RTS, delay_rts_before_send and delay_rts_after_send in RS485 mode.
> Changes for V3:
> Modify optional gpio name to rts-gpios.
> Update commit description.
> Move cdns_uart_tx_empty function to avoid prototype statement.
> Remove assignment of struct serial_rs485 to port->rs485 as
> serial core performs that.
> Switch to native RTS in non GPIO case.
> Handle rs485 during stop tx.
> Remove explicit calls to configure gpio direction and value,
> as devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW argument.
> Update implementation to support configuration of GPIO/RTS value
> based on user configuration of SER_RS485_RTS_ON_SEND and
> SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from handle_tx.
> ---
>  drivers/tty/serial/xilinx_uartps.c | 180 ++++++++++++++++++++++++++---
>  1 file changed, 165 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 9c13dac1d4d1..32229cf5c508 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -23,6 +23,9 @@
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/iopoll.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>  
>  #define CDNS_UART_TTY_NAME	"ttyPS"
>  #define CDNS_UART_NAME		"xuartps"
> @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
>   * @clk_rate_change_nb:	Notifier block for clock changes
>   * @quirks:		Flags for RXBS support.
>   * @cts_override:	Modem control state override
> + * @gpiod:		Pointer to the gpio descriptor
>   */
>  struct cdns_uart {
>  	struct uart_port	*port;
> @@ -203,10 +207,19 @@ struct cdns_uart {
>  	struct notifier_block	clk_rate_change_nb;
>  	u32			quirks;
>  	bool cts_override;
> +	struct gpio_desc	*gpiod;
>  };
>  struct cdns_platform_data {
>  	u32 quirks;
>  };
> +
> +struct serial_rs485 cdns_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_before_send = 1,
> +	.delay_rts_after_send = 1,
> +};
> +
>  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
>  		clk_rate_change_nb)
>  
> @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
>  	tty_flip_buffer_push(&port->state->port);
>  }
>  
> +/**
> + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_high(struct cdns_uart *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 1);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val &= ~CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_low(struct cdns_uart *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 0);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val |= CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_tx_setup - Tx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_tx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_rs485_rx_setup - Rx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_rx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_uart_tx_empty -  Check whether TX is empty
> + * @port: Handle to the uart port structure
> + *
> + * Return: TIOCSER_TEMT on success, 0 otherwise
> + */
> +static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> +{
> +	unsigned int status;
> +
> +	status = readl(port->membase + CDNS_UART_SR) &
> +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);

Split to two lines since you need two lines anyway.

> +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> +}
> +
>  /**
>   * cdns_uart_handle_tx - Handle the bytes to be Txed.
>   * @dev_id: Id of the UART port
> @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
>  static void cdns_uart_start_tx(struct uart_port *port)
>  {
>  	unsigned int status;
> +	unsigned long time_out;
> +	struct cdns_uart *cdns_uart = port->private_data;
>  
>  	if (uart_tx_stopped(port))
>  		return;
> @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port *port)
>  
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
>  
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		cdns_rs485_tx_setup(cdns_uart);
> +		if (cdns_uart->port->rs485.delay_rts_before_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_before_send);
> +	}
> +
>  	cdns_uart_handle_tx(port);
>  
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> +		/* Wait for tx completion */
> +		while ((cdns_uart_tx_empty(cdns_uart->port) != TIOCSER_TEMT) &&
> +		       time_before(jiffies, time_out))
> +			cpu_relax();

Use iopoll.h helper instead of handcrafted delay loop ?

> +
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		/*
> +		 * Default Rx should be setup, because RX signaling path
> +		 * need to enable to receive data.
> +		 */
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}
> +
>  	/* Enable the TX Empty interrupt */
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
>  }
> @@ -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
>  static void cdns_uart_stop_tx(struct uart_port *port)
>  {
>  	unsigned int regval;
> +	struct cdns_uart *cdns_uart = port->private_data;
> +
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}
>  
>  	regval = readl(port->membase + CDNS_UART_CR);
>  	regval |= CDNS_UART_CR_TX_DIS;
> @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port *port)
>  	writel(regval, port->membase + CDNS_UART_CR);
>  }
>  
> -/**
> - * cdns_uart_tx_empty -  Check whether TX is empty
> - * @port: Handle to the uart port structure
> - *
> - * Return: TIOCSER_TEMT on success, 0 otherwise
> - */
> -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> -{
> -	unsigned int status;
> -
> -	status = readl(port->membase + CDNS_UART_SR) &
> -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> -}

This is just a relocation of code? Move it in another patch in the 
series, don't put it within the feature patch..

> -
>  /**
>   * cdns_uart_break_ctl - Based on the input ctl we have to start or stop
>   *			transmitting char breaks
> @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port *port)
>  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
>  		cpu_relax();
>  
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> +		cdns_rs485_rx_setup(cdns_uart);
> +
>  	/*
>  	 * Clear the RX disable bit and then set the RX enable bit to enable
>  	 * the receiver.
> @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
>  /* Temporary variable for storing number of instances */
>  static int instances;
>  
> +/**
> + * cdns_rs485_config - Called when an application calls TIOCSRS485 ioctl.
> + * @port: Pointer to the uart_port structure
> + * @termios: Pointer to the ktermios structure
> + * @rs485: Pointer to the serial_rs485 structure
> + *
> + * Return: 0
> + */
> +static int cdns_rs485_config(struct uart_port *port, struct ktermios *termios,
> +			     struct serial_rs485 *rs485)
> +{
> +	if (rs485->flags & SER_RS485_ENABLED)
> +		dev_dbg(port->dev, "Setting UART to RS485\n");
> +
> +	return 0;
> +}
> +
>  /**
>   * cdns_uart_probe - Platform driver probe
>   * @pdev: Pointer to the platform device structure
> @@ -1463,6 +1587,7 @@ static int instances;
>   */
>  static int cdns_uart_probe(struct platform_device *pdev)
>  {
> +	u32 val;
>  	int rc, id, irq;
>  	struct uart_port *port;
>  	struct resource *res;
> @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	port->private_data = cdns_uart_data;
>  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG |
>  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> +	port->rs485_config = cdns_rs485_config;
> +	port->rs485_supported = cdns_rs485_supported;
>  	cdns_uart_data->port = port;
>  	platform_set_drvdata(pdev, port);
>  
> +	rc = uart_get_rs485_mode(port);
> +	if (rc)
> +		goto err_out_clk_notifier;
> +
> +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev, "rts",
> +							GPIOD_OUT_LOW);
> +	if (IS_ERR(cdns_uart_data->gpiod)) {
> +		rc = PTR_ERR(cdns_uart_data->gpiod);
> +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional failed\n");
> +		goto err_out_clk_notifier;
> +	}
> +
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_set_active(&pdev->dev);
> @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
>  							     "cts-override");
>  
> +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (!cdns_uart_data->gpiod) {

Combine the if conditions into a single if.

> +			val = readl(cdns_uart_data->port->membase
> +				    + CDNS_UART_MODEMCR);

One line.

> +			val |= CDNS_UART_MODEMCR_RTS;
> +			writel(val, cdns_uart_data->port->membase
> +			       + CDNS_UART_MODEMCR);

Ditto.

> +		}
> +	}
> +
>  	instances++;
>  
>  	return 0;
> @@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_set_suspended(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +err_out_clk_notifier:
>  #ifdef CONFIG_COMMON_CLK
>  	clk_notifier_unregister(cdns_uart_data->uartclk,
>  			&cdns_uart_data->clk_rate_change_nb);
> 

-- 
 i.

Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Ilpo Järvinen]

On Tue, 24 Oct 2023, Manikanta Guntupalli wrote:

> Add rs485 support to uartps driver. Use either rts-gpios or RTS
> to control RS485 phy as driver or a receiver.
> 
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Modify optional gpio name to xlnx,phy-ctrl-gpios.
> Update commit description.
> Add support for RTS, delay_rts_before_send and delay_rts_after_send in RS485 mode.
> Changes for V3:
> Modify optional gpio name to rts-gpios.
> Update commit description.
> Move cdns_uart_tx_empty function to avoid prototype statement.
> Remove assignment of struct serial_rs485 to port->rs485 as
> serial core performs that.
> Switch to native RTS in non GPIO case.
> Handle rs485 during stop tx.
> Remove explicit calls to configure gpio direction and value,
> as devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW argument.
> Update implementation to support configuration of GPIO/RTS value
> based on user configuration of SER_RS485_RTS_ON_SEND and
> SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from handle_tx.
> ---
>  drivers/tty/serial/xilinx_uartps.c | 180 ++++++++++++++++++++++++++---
>  1 file changed, 165 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 9c13dac1d4d1..32229cf5c508 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -23,6 +23,9 @@
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/iopoll.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>  
>  #define CDNS_UART_TTY_NAME	"ttyPS"
>  #define CDNS_UART_NAME		"xuartps"
> @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
>   * @clk_rate_change_nb:	Notifier block for clock changes
>   * @quirks:		Flags for RXBS support.
>   * @cts_override:	Modem control state override
> + * @gpiod:		Pointer to the gpio descriptor
>   */
>  struct cdns_uart {
>  	struct uart_port	*port;
> @@ -203,10 +207,19 @@ struct cdns_uart {
>  	struct notifier_block	clk_rate_change_nb;
>  	u32			quirks;
>  	bool cts_override;
> +	struct gpio_desc	*gpiod;
>  };
>  struct cdns_platform_data {
>  	u32 quirks;
>  };
> +
> +struct serial_rs485 cdns_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_before_send = 1,
> +	.delay_rts_after_send = 1,
> +};
> +
>  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
>  		clk_rate_change_nb)
>  
> @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
>  	tty_flip_buffer_push(&port->state->port);
>  }
>  
> +/**
> + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_high(struct cdns_uart *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 1);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val &= ~CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_low(struct cdns_uart *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 0);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val |= CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_tx_setup - Tx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_tx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_rs485_rx_setup - Rx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_rx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_uart_tx_empty -  Check whether TX is empty
> + * @port: Handle to the uart port structure
> + *
> + * Return: TIOCSER_TEMT on success, 0 otherwise
> + */
> +static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> +{
> +	unsigned int status;
> +
> +	status = readl(port->membase + CDNS_UART_SR) &
> +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);

Split to two lines since you need two lines anyway.

> +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> +}
> +
>  /**
>   * cdns_uart_handle_tx - Handle the bytes to be Txed.
>   * @dev_id: Id of the UART port
> @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
>  static void cdns_uart_start_tx(struct uart_port *port)
>  {
>  	unsigned int status;
> +	unsigned long time_out;
> +	struct cdns_uart *cdns_uart = port->private_data;
>  
>  	if (uart_tx_stopped(port))
>  		return;
> @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port *port)
>  
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
>  
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		cdns_rs485_tx_setup(cdns_uart);
> +		if (cdns_uart->port->rs485.delay_rts_before_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_before_send);
> +	}
> +
>  	cdns_uart_handle_tx(port);
>  
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> +		/* Wait for tx completion */
> +		while ((cdns_uart_tx_empty(cdns_uart->port) != TIOCSER_TEMT) &&
> +		       time_before(jiffies, time_out))
> +			cpu_relax();

Use iopoll.h helper instead of handcrafted delay loop ?

> +
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		/*
> +		 * Default Rx should be setup, because RX signaling path
> +		 * need to enable to receive data.
> +		 */
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}
> +
>  	/* Enable the TX Empty interrupt */
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
>  }
> @@ -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
>  static void cdns_uart_stop_tx(struct uart_port *port)
>  {
>  	unsigned int regval;
> +	struct cdns_uart *cdns_uart = port->private_data;
> +
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}
>  
>  	regval = readl(port->membase + CDNS_UART_CR);
>  	regval |= CDNS_UART_CR_TX_DIS;
> @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port *port)
>  	writel(regval, port->membase + CDNS_UART_CR);
>  }
>  
> -/**
> - * cdns_uart_tx_empty -  Check whether TX is empty
> - * @port: Handle to the uart port structure
> - *
> - * Return: TIOCSER_TEMT on success, 0 otherwise
> - */
> -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> -{
> -	unsigned int status;
> -
> -	status = readl(port->membase + CDNS_UART_SR) &
> -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> -}

This is just a relocation of code? Move it in another patch in the 
series, don't put it within the feature patch..

> -
>  /**
>   * cdns_uart_break_ctl - Based on the input ctl we have to start or stop
>   *			transmitting char breaks
> @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port *port)
>  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
>  		cpu_relax();
>  
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> +		cdns_rs485_rx_setup(cdns_uart);
> +
>  	/*
>  	 * Clear the RX disable bit and then set the RX enable bit to enable
>  	 * the receiver.
> @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
>  /* Temporary variable for storing number of instances */
>  static int instances;
>  
> +/**
> + * cdns_rs485_config - Called when an application calls TIOCSRS485 ioctl.
> + * @port: Pointer to the uart_port structure
> + * @termios: Pointer to the ktermios structure
> + * @rs485: Pointer to the serial_rs485 structure
> + *
> + * Return: 0
> + */
> +static int cdns_rs485_config(struct uart_port *port, struct ktermios *termios,
> +			     struct serial_rs485 *rs485)
> +{
> +	if (rs485->flags & SER_RS485_ENABLED)
> +		dev_dbg(port->dev, "Setting UART to RS485\n");
> +
> +	return 0;
> +}
> +
>  /**
>   * cdns_uart_probe - Platform driver probe
>   * @pdev: Pointer to the platform device structure
> @@ -1463,6 +1587,7 @@ static int instances;
>   */
>  static int cdns_uart_probe(struct platform_device *pdev)
>  {
> +	u32 val;
>  	int rc, id, irq;
>  	struct uart_port *port;
>  	struct resource *res;
> @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	port->private_data = cdns_uart_data;
>  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG |
>  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> +	port->rs485_config = cdns_rs485_config;
> +	port->rs485_supported = cdns_rs485_supported;
>  	cdns_uart_data->port = port;
>  	platform_set_drvdata(pdev, port);
>  
> +	rc = uart_get_rs485_mode(port);
> +	if (rc)
> +		goto err_out_clk_notifier;
> +
> +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev, "rts",
> +							GPIOD_OUT_LOW);
> +	if (IS_ERR(cdns_uart_data->gpiod)) {
> +		rc = PTR_ERR(cdns_uart_data->gpiod);
> +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional failed\n");
> +		goto err_out_clk_notifier;
> +	}
> +
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_set_active(&pdev->dev);
> @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
>  							     "cts-override");
>  
> +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (!cdns_uart_data->gpiod) {

Combine the if conditions into a single if.

> +			val = readl(cdns_uart_data->port->membase
> +				    + CDNS_UART_MODEMCR);

One line.

> +			val |= CDNS_UART_MODEMCR_RTS;
> +			writel(val, cdns_uart_data->port->membase
> +			       + CDNS_UART_MODEMCR);

Ditto.

> +		}
> +	}
> +
>  	instances++;
>  
>  	return 0;
> @@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_set_suspended(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +err_out_clk_notifier:
>  #ifdef CONFIG_COMMON_CLK
>  	clk_notifier_unregister(cdns_uart_data->uartclk,
>  			&cdns_uart_data->clk_rate_change_nb);
> 

-- 
 i.


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

Re: [PATCH V3 1/2] dt-bindings: Add optional gpio property to uartps node to support rs485

[Rob Herring]

On Tue, Oct 24, 2023 at 08:18:46PM +0530, Manikanta Guntupalli wrote:
> Add optional gpio property to uartps node and reference to rs485.yaml
> 
> On Xilinx/AMD Kria SOM KD240 board rs485 connects via TI ISOW1432
> Transceiver device, where one GPIO is used for driving DE/RE signals.
> With rs485 half duplex configuration, DE and RE are shorted to each other,
> and at a time, any node acts as either a driver or a receiver.
> 
> Here,
> DE - Driver enable. If pin is floating, driver is disabled.
> RE - Receiver enable. If pin is floating, receiver buffer is disabled.

What happens when pin is not floating? Is floating (i.e. open drain) for 
RTS a requirement? And floating doesn't define high or low because it 
could be pulled either way.

> 
> For more deatils, please find below link which contains Transceiver
> device(ISOW1432) datasheet
> https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN
> 
> rts-gpios is optional property, because it is not required
> for uart console node.
> 
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Modify optional gpio name to xlnx,phy-ctrl-gpios.
> Update commit description.
> Changes for V3:
> Modify optional gpio name to rts-gpios.
> Update commit description.
> ---
>  Documentation/devicetree/bindings/serial/cdns,uart.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> index e35ad1109efc..7ee305f9a45f 100644
> --- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> @@ -46,6 +46,11 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  rts-gpios:
> +    description: Optional GPIO to control transmit/receive on RS485 phy
> +      in halfduplex mode.

You need to define what 'active' means here because -gpios all have an 
active flag. Is it always active low (or high) or depends on the board?


> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -55,6 +60,7 @@ required:
>  
>  allOf:
>    - $ref: serial.yaml#
> +  - $ref: rs485.yaml#
>    - if:
>        properties:
>          compatible:
> -- 
> 2.25.1
> 

Re: [PATCH V3 1/2] dt-bindings: Add optional gpio property to uartps node to support rs485

[Rob Herring]

On Tue, Oct 24, 2023 at 08:18:46PM +0530, Manikanta Guntupalli wrote:
> Add optional gpio property to uartps node and reference to rs485.yaml
> 
> On Xilinx/AMD Kria SOM KD240 board rs485 connects via TI ISOW1432
> Transceiver device, where one GPIO is used for driving DE/RE signals.
> With rs485 half duplex configuration, DE and RE are shorted to each other,
> and at a time, any node acts as either a driver or a receiver.
> 
> Here,
> DE - Driver enable. If pin is floating, driver is disabled.
> RE - Receiver enable. If pin is floating, receiver buffer is disabled.

What happens when pin is not floating? Is floating (i.e. open drain) for 
RTS a requirement? And floating doesn't define high or low because it 
could be pulled either way.

> 
> For more deatils, please find below link which contains Transceiver
> device(ISOW1432) datasheet
> https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN
> 
> rts-gpios is optional property, because it is not required
> for uart console node.
> 
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Modify optional gpio name to xlnx,phy-ctrl-gpios.
> Update commit description.
> Changes for V3:
> Modify optional gpio name to rts-gpios.
> Update commit description.
> ---
>  Documentation/devicetree/bindings/serial/cdns,uart.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> index e35ad1109efc..7ee305f9a45f 100644
> --- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> @@ -46,6 +46,11 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  rts-gpios:
> +    description: Optional GPIO to control transmit/receive on RS485 phy
> +      in halfduplex mode.

You need to define what 'active' means here because -gpios all have an 
active flag. Is it always active low (or high) or depends on the board?


> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> @@ -55,6 +60,7 @@ required:
>  
>  allOf:
>    - $ref: serial.yaml#
> +  - $ref: rs485.yaml#
>    - if:
>        properties:
>          compatible:
> -- 
> 2.25.1
> 

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

Re: [PATCH V3 1/2] dt-bindings: Add optional gpio property to uartps node to support rs485

[m.brock]

Manikanta Guntupalli schreef op 2023-10-24 16:48:
> Add optional gpio property to uartps node and reference to rs485.yaml
> 
> On Xilinx/AMD Kria SOM KD240 board rs485 connects via TI ISOW1432
> Transceiver device, where one GPIO is used for driving DE/RE signals.
> With rs485 half duplex configuration, DE and RE are shorted to each 
> other,
> and at a time, any node acts as either a driver or a receiver.
> 
> Here,
> DE - Driver enable. If pin is floating, driver is disabled.
> RE - Receiver enable. If pin is floating, receiver buffer is disabled.
> 
> For more deatils, please find below link which contains Transceiver
> device(ISOW1432) datasheet
> https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN
> 
> rts-gpios is optional property, because it is not required
> for uart console node.
> 
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Modify optional gpio name to xlnx,phy-ctrl-gpios.
> Update commit description.
> Changes for V3:
> Modify optional gpio name to rts-gpios.
> Update commit description.
> ---
>  Documentation/devicetree/bindings/serial/cdns,uart.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> index e35ad1109efc..7ee305f9a45f 100644
> --- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> @@ -46,6 +46,11 @@ properties:
>    power-domains:
>      maxItems: 1
> 
> +  rts-gpios:
> +    description: Optional GPIO to control transmit/receive on RS485 
> phy
> +      in halfduplex mode.
> +    maxItems: 1
> +

Why would this be related to RS485? A user could also have a need for a
gpio instead of the native pin to be used as normal rts.
All RS485 references can be removed.

>  required:
>    - compatible
>    - reg
> @@ -55,6 +60,7 @@ required:
> 
>  allOf:
>    - $ref: serial.yaml#
> +  - $ref: rs485.yaml#
>    - if:
>        properties:
>          compatible:

Maarten

Re: [PATCH V3 1/2] dt-bindings: Add optional gpio property to uartps node to support rs485

[m.brock]

Manikanta Guntupalli schreef op 2023-10-24 16:48:
> Add optional gpio property to uartps node and reference to rs485.yaml
> 
> On Xilinx/AMD Kria SOM KD240 board rs485 connects via TI ISOW1432
> Transceiver device, where one GPIO is used for driving DE/RE signals.
> With rs485 half duplex configuration, DE and RE are shorted to each 
> other,
> and at a time, any node acts as either a driver or a receiver.
> 
> Here,
> DE - Driver enable. If pin is floating, driver is disabled.
> RE - Receiver enable. If pin is floating, receiver buffer is disabled.
> 
> For more deatils, please find below link which contains Transceiver
> device(ISOW1432) datasheet
> https://www.ti.com/lit/ds/symlink/isow1432.pdf?ts=1682607122706&ref_url=https%253A%252F%252Fwww.ti.com%252Fproduct%252FISOW1432%252Fpart-details%252FISOW1432DFMR%253FkeyMatch%253DISOW1432DFMR%2526tisearch%253Dsearch-everything%2526usecase%253DOPN
> 
> rts-gpios is optional property, because it is not required
> for uart console node.
> 
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Modify optional gpio name to xlnx,phy-ctrl-gpios.
> Update commit description.
> Changes for V3:
> Modify optional gpio name to rts-gpios.
> Update commit description.
> ---
>  Documentation/devicetree/bindings/serial/cdns,uart.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> index e35ad1109efc..7ee305f9a45f 100644
> --- a/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> +++ b/Documentation/devicetree/bindings/serial/cdns,uart.yaml
> @@ -46,6 +46,11 @@ properties:
>    power-domains:
>      maxItems: 1
> 
> +  rts-gpios:
> +    description: Optional GPIO to control transmit/receive on RS485 
> phy
> +      in halfduplex mode.
> +    maxItems: 1
> +

Why would this be related to RS485? A user could also have a need for a
gpio instead of the native pin to be used as normal rts.
All RS485 references can be removed.

>  required:
>    - compatible
>    - reg
> @@ -55,6 +60,7 @@ required:
> 
>  allOf:
>    - $ref: serial.yaml#
> +  - $ref: rs485.yaml#
>    - if:
>        properties:
>          compatible:

Maarten


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

Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[m.brock]

Manikanta Guntupalli wrote on 2023-10-24 16:48:
> @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
>   * @clk_rate_change_nb:	Notifier block for clock changes
>   * @quirks:		Flags for RXBS support.
>   * @cts_override:	Modem control state override
> + * @gpiod:		Pointer to the gpio descriptor

Change gpiod to gpiod_rts maybe?
Later someone might want to use a gpio for cts/dtr/dsr/dcd/ri as well.

>   */
>  struct cdns_uart {
>  	struct uart_port	*port;
> @@ -203,10 +207,19 @@ struct cdns_uart {
>  	struct notifier_block	clk_rate_change_nb;
>  	u32			quirks;
>  	bool cts_override;
> +	struct gpio_desc	*gpiod;
>  };
>  struct cdns_platform_data {
>  	u32 quirks;
>  };
> +
> +struct serial_rs485 cdns_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_before_send = 1,
> +	.delay_rts_after_send = 1,
> +};
> +
>  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
>  		clk_rate_change_nb)
> 
> @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id,
> unsigned int isrstatus)
>  	tty_flip_buffer_push(&port->state->port);
>  }
> 
> +/**
> + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_high(struct cdns_uart 
> *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 1);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val &= ~CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_low(struct cdns_uart 
> *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 0);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val |= CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_tx_setup - Tx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_tx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_rs485_rx_setup - Rx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_rx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}

Why not simply create:
void cdns_rs485_driver_enable(struct cdns_uart *cdns_uart, bool enable)

And let it handle the rs485.flags itself?

> +
> +/**
> + * cdns_uart_tx_empty -  Check whether TX is empty
> + * @port: Handle to the uart port structure
> + *
> + * Return: TIOCSER_TEMT on success, 0 otherwise
> + */
> +static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> +{
> +	unsigned int status;
> +
> +	status = readl(port->membase + CDNS_UART_SR) &
> +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> +}
> +
>  /**
>   * cdns_uart_handle_tx - Handle the bytes to be Txed.
>   * @dev_id: Id of the UART port
> @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct
> notifier_block *nb,
>  static void cdns_uart_start_tx(struct uart_port *port)
>  {
>  	unsigned int status;
> +	unsigned long time_out;
> +	struct cdns_uart *cdns_uart = port->private_data;
> 
>  	if (uart_tx_stopped(port))
>  		return;
> @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port 
> *port)
> 
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		cdns_rs485_tx_setup(cdns_uart);
> +		if (cdns_uart->port->rs485.delay_rts_before_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_before_send);

Would it not be better to start a timer here with a callback that 
enables
the TXEMPTY interrupt? That would automatically call 
cdns_uart_handle_tx().

> +	}
> +
>  	cdns_uart_handle_tx(port);
> 
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> +		/* Wait for tx completion */
> +		while ((cdns_uart_tx_empty(cdns_uart->port) != TIOCSER_TEMT) &&
> +		       time_before(jiffies, time_out))
> +			cpu_relax();
> +
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		/*
> +		 * Default Rx should be setup, because RX signaling path
> +		 * need to enable to receive data.
> +		 */
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}

I think this should be done from the TXEMPTY interrupt. And again 
schedule a
timer to drop the DE line. You really can do this without using 
mdelay().

> +
>  	/* Enable the TX Empty interrupt */
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
>  }
> @@ -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port 
> *port)
>  static void cdns_uart_stop_tx(struct uart_port *port)
>  {
>  	unsigned int regval;
> +	struct cdns_uart *cdns_uart = port->private_data;
> +
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}

Again, start a timer and wait for completion?

>  	regval = readl(port->membase + CDNS_UART_CR);
>  	regval |= CDNS_UART_CR_TX_DIS;
> @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port 
> *port)
>  	writel(regval, port->membase + CDNS_UART_CR);
>  }
> 
> -/**
> - * cdns_uart_tx_empty -  Check whether TX is empty
> - * @port: Handle to the uart port structure
> - *
> - * Return: TIOCSER_TEMT on success, 0 otherwise
> - */
> -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> -{
> -	unsigned int status;
> -
> -	status = readl(port->membase + CDNS_UART_SR) &
> -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> -}
> -
>  /**
>   * cdns_uart_break_ctl - Based on the input ctl we have to start or 
> stop
>   *			transmitting char breaks
> @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port 
> *port)
>  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
>  		cpu_relax();
> 
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> +		cdns_rs485_rx_setup(cdns_uart);
> +
>  	/*
>  	 * Clear the RX disable bit and then set the RX enable bit to enable
>  	 * the receiver.
> @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
>  /* Temporary variable for storing number of instances */
>  static int instances;
> 
> +/**
> + * cdns_rs485_config - Called when an application calls TIOCSRS485 
> ioctl.
> + * @port: Pointer to the uart_port structure
> + * @termios: Pointer to the ktermios structure
> + * @rs485: Pointer to the serial_rs485 structure
> + *
> + * Return: 0
> + */
> +static int cdns_rs485_config(struct uart_port *port, struct ktermios 
> *termios,
> +			     struct serial_rs485 *rs485)
> +{
> +	if (rs485->flags & SER_RS485_ENABLED)
> +		dev_dbg(port->dev, "Setting UART to RS485\n");
> +

Shouldn't you force automatic RTS control off here?
And also call cdns_rs485_rx_setup()

> +	return 0;
> +}
> +
>  /**
>   * cdns_uart_probe - Platform driver probe
>   * @pdev: Pointer to the platform device structure
> @@ -1463,6 +1587,7 @@ static int instances;
>   */
>  static int cdns_uart_probe(struct platform_device *pdev)
>  {
> +	u32 val;
>  	int rc, id, irq;
>  	struct uart_port *port;
>  	struct resource *res;
> @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct 
> platform_device *pdev)
>  	port->private_data = cdns_uart_data;
>  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG 
> |
>  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> +	port->rs485_config = cdns_rs485_config;
> +	port->rs485_supported = cdns_rs485_supported;
>  	cdns_uart_data->port = port;
>  	platform_set_drvdata(pdev, port);
> 
> +	rc = uart_get_rs485_mode(port);
> +	if (rc)
> +		goto err_out_clk_notifier;
> +
> +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev, "rts",
> +							GPIOD_OUT_LOW);
> +	if (IS_ERR(cdns_uart_data->gpiod)) {
> +		rc = PTR_ERR(cdns_uart_data->gpiod);
> +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional failed\n");
> +		goto err_out_clk_notifier;
> +	}
> +
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 
> UART_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_set_active(&pdev->dev);
> @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct 
> platform_device *pdev)
>  	cdns_uart_data->cts_override = 
> of_property_read_bool(pdev->dev.of_node,
>  							     "cts-override");
> 
> +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (!cdns_uart_data->gpiod) {
> +			val = readl(cdns_uart_data->port->membase
> +				    + CDNS_UART_MODEMCR);
> +			val |= CDNS_UART_MODEMCR_RTS;
> +			writel(val, cdns_uart_data->port->membase
> +			       + CDNS_UART_MODEMCR);
> +		}
> +	}

Simply call cdns_rs485_rx_setup() ?

> +
>  	instances++;
> 
>  	return 0;
> @@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device 
> *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_set_suspended(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +err_out_clk_notifier:
>  #ifdef CONFIG_COMMON_CLK
>  	clk_notifier_unregister(cdns_uart_data->uartclk,
>  			&cdns_uart_data->clk_rate_change_nb);

Please also modify cdns_uart_[s|g]et_mctrl() so they support rts-gpios.

Maarten

Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[m.brock]

Manikanta Guntupalli wrote on 2023-10-24 16:48:
> @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
>   * @clk_rate_change_nb:	Notifier block for clock changes
>   * @quirks:		Flags for RXBS support.
>   * @cts_override:	Modem control state override
> + * @gpiod:		Pointer to the gpio descriptor

Change gpiod to gpiod_rts maybe?
Later someone might want to use a gpio for cts/dtr/dsr/dcd/ri as well.

>   */
>  struct cdns_uart {
>  	struct uart_port	*port;
> @@ -203,10 +207,19 @@ struct cdns_uart {
>  	struct notifier_block	clk_rate_change_nb;
>  	u32			quirks;
>  	bool cts_override;
> +	struct gpio_desc	*gpiod;
>  };
>  struct cdns_platform_data {
>  	u32 quirks;
>  };
> +
> +struct serial_rs485 cdns_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_before_send = 1,
> +	.delay_rts_after_send = 1,
> +};
> +
>  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
>  		clk_rate_change_nb)
> 
> @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id,
> unsigned int isrstatus)
>  	tty_flip_buffer_push(&port->state->port);
>  }
> 
> +/**
> + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_high(struct cdns_uart 
> *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 1);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val &= ~CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_low(struct cdns_uart 
> *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 0);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val |= CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_tx_setup - Tx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_tx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_rs485_rx_setup - Rx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_rx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}

Why not simply create:
void cdns_rs485_driver_enable(struct cdns_uart *cdns_uart, bool enable)

And let it handle the rs485.flags itself?

> +
> +/**
> + * cdns_uart_tx_empty -  Check whether TX is empty
> + * @port: Handle to the uart port structure
> + *
> + * Return: TIOCSER_TEMT on success, 0 otherwise
> + */
> +static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> +{
> +	unsigned int status;
> +
> +	status = readl(port->membase + CDNS_UART_SR) &
> +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> +}
> +
>  /**
>   * cdns_uart_handle_tx - Handle the bytes to be Txed.
>   * @dev_id: Id of the UART port
> @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct
> notifier_block *nb,
>  static void cdns_uart_start_tx(struct uart_port *port)
>  {
>  	unsigned int status;
> +	unsigned long time_out;
> +	struct cdns_uart *cdns_uart = port->private_data;
> 
>  	if (uart_tx_stopped(port))
>  		return;
> @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port 
> *port)
> 
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		cdns_rs485_tx_setup(cdns_uart);
> +		if (cdns_uart->port->rs485.delay_rts_before_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_before_send);

Would it not be better to start a timer here with a callback that 
enables
the TXEMPTY interrupt? That would automatically call 
cdns_uart_handle_tx().

> +	}
> +
>  	cdns_uart_handle_tx(port);
> 
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> +		/* Wait for tx completion */
> +		while ((cdns_uart_tx_empty(cdns_uart->port) != TIOCSER_TEMT) &&
> +		       time_before(jiffies, time_out))
> +			cpu_relax();
> +
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		/*
> +		 * Default Rx should be setup, because RX signaling path
> +		 * need to enable to receive data.
> +		 */
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}

I think this should be done from the TXEMPTY interrupt. And again 
schedule a
timer to drop the DE line. You really can do this without using 
mdelay().

> +
>  	/* Enable the TX Empty interrupt */
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
>  }
> @@ -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port 
> *port)
>  static void cdns_uart_stop_tx(struct uart_port *port)
>  {
>  	unsigned int regval;
> +	struct cdns_uart *cdns_uart = port->private_data;
> +
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}

Again, start a timer and wait for completion?

>  	regval = readl(port->membase + CDNS_UART_CR);
>  	regval |= CDNS_UART_CR_TX_DIS;
> @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port 
> *port)
>  	writel(regval, port->membase + CDNS_UART_CR);
>  }
> 
> -/**
> - * cdns_uart_tx_empty -  Check whether TX is empty
> - * @port: Handle to the uart port structure
> - *
> - * Return: TIOCSER_TEMT on success, 0 otherwise
> - */
> -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> -{
> -	unsigned int status;
> -
> -	status = readl(port->membase + CDNS_UART_SR) &
> -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> -}
> -
>  /**
>   * cdns_uart_break_ctl - Based on the input ctl we have to start or 
> stop
>   *			transmitting char breaks
> @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port 
> *port)
>  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
>  		cpu_relax();
> 
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> +		cdns_rs485_rx_setup(cdns_uart);
> +
>  	/*
>  	 * Clear the RX disable bit and then set the RX enable bit to enable
>  	 * the receiver.
> @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
>  /* Temporary variable for storing number of instances */
>  static int instances;
> 
> +/**
> + * cdns_rs485_config - Called when an application calls TIOCSRS485 
> ioctl.
> + * @port: Pointer to the uart_port structure
> + * @termios: Pointer to the ktermios structure
> + * @rs485: Pointer to the serial_rs485 structure
> + *
> + * Return: 0
> + */
> +static int cdns_rs485_config(struct uart_port *port, struct ktermios 
> *termios,
> +			     struct serial_rs485 *rs485)
> +{
> +	if (rs485->flags & SER_RS485_ENABLED)
> +		dev_dbg(port->dev, "Setting UART to RS485\n");
> +

Shouldn't you force automatic RTS control off here?
And also call cdns_rs485_rx_setup()

> +	return 0;
> +}
> +
>  /**
>   * cdns_uart_probe - Platform driver probe
>   * @pdev: Pointer to the platform device structure
> @@ -1463,6 +1587,7 @@ static int instances;
>   */
>  static int cdns_uart_probe(struct platform_device *pdev)
>  {
> +	u32 val;
>  	int rc, id, irq;
>  	struct uart_port *port;
>  	struct resource *res;
> @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct 
> platform_device *pdev)
>  	port->private_data = cdns_uart_data;
>  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG 
> |
>  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> +	port->rs485_config = cdns_rs485_config;
> +	port->rs485_supported = cdns_rs485_supported;
>  	cdns_uart_data->port = port;
>  	platform_set_drvdata(pdev, port);
> 
> +	rc = uart_get_rs485_mode(port);
> +	if (rc)
> +		goto err_out_clk_notifier;
> +
> +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev, "rts",
> +							GPIOD_OUT_LOW);
> +	if (IS_ERR(cdns_uart_data->gpiod)) {
> +		rc = PTR_ERR(cdns_uart_data->gpiod);
> +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional failed\n");
> +		goto err_out_clk_notifier;
> +	}
> +
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 
> UART_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_set_active(&pdev->dev);
> @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct 
> platform_device *pdev)
>  	cdns_uart_data->cts_override = 
> of_property_read_bool(pdev->dev.of_node,
>  							     "cts-override");
> 
> +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (!cdns_uart_data->gpiod) {
> +			val = readl(cdns_uart_data->port->membase
> +				    + CDNS_UART_MODEMCR);
> +			val |= CDNS_UART_MODEMCR_RTS;
> +			writel(val, cdns_uart_data->port->membase
> +			       + CDNS_UART_MODEMCR);
> +		}
> +	}

Simply call cdns_rs485_rx_setup() ?

> +
>  	instances++;
> 
>  	return 0;
> @@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device 
> *pdev)
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_set_suspended(&pdev->dev);
>  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> +err_out_clk_notifier:
>  #ifdef CONFIG_COMMON_CLK
>  	clk_notifier_unregister(cdns_uart_data->uartclk,
>  			&cdns_uart_data->clk_rate_change_nb);

Please also modify cdns_uart_[s|g]et_mctrl() so they support rts-gpios.

Maarten


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

RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Guntupalli, Manikanta]

Hi,

> -----Original Message-----
> From: m.brock@vanmierlo.com <m.brock@vanmierlo.com>
> Sent: Saturday, November 4, 2023 9:17 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
> Cc: git (AMD-Xilinx) <git@amd.com>; Simek, Michal
> <michal.simek@amd.com>; gregkh@linuxfoundation.org;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; linux-serial@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> jirislaby@kernel.org; linux-arm-kernel@lists.infradead.org; Pandey, Radhey
> Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; manion05gk@gmail.com
> Subject: Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps
> driver
> 
> Manikanta Guntupalli wrote on 2023-10-24 16:48:
> > @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-
> 255");
> >   * @clk_rate_change_nb:	Notifier block for clock changes
> >   * @quirks:		Flags for RXBS support.
> >   * @cts_override:	Modem control state override
> > + * @gpiod:		Pointer to the gpio descriptor
> 
> Change gpiod to gpiod_rts maybe?
> Later someone might want to use a gpio for cts/dtr/dsr/dcd/ri as well.
We will fix.
> 
> >   */
> >  struct cdns_uart {
> >  	struct uart_port	*port;
> > @@ -203,10 +207,19 @@ struct cdns_uart {
> >  	struct notifier_block	clk_rate_change_nb;
> >  	u32			quirks;
> >  	bool cts_override;
> > +	struct gpio_desc	*gpiod;
> >  };
> >  struct cdns_platform_data {
> >  	u32 quirks;
> >  };
> > +
> > +struct serial_rs485 cdns_rs485_supported = {
> > +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> > +		 SER_RS485_RTS_AFTER_SEND,
> > +	.delay_rts_before_send = 1,
> > +	.delay_rts_after_send = 1,
> > +};
> > +
> >  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
> >  		clk_rate_change_nb)
> >
> > @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id,
> > unsigned int isrstatus)
> >  	tty_flip_buffer_push(&port->state->port);
> >  }
> >
> > +/**
> > + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_high(struct cdns_uart
> > *cdns_uart)
> > +{
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 1);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val &= ~CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_low(struct cdns_uart
> > *cdns_uart)
> > +{
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 0);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val |= CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_tx_setup - Tx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_tx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> > +
> > +/**
> > + * cdns_rs485_rx_setup - Rx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_rx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> 
> Why not simply create:
> void cdns_rs485_driver_enable(struct cdns_uart *cdns_uart, bool enable)
We will fix.
> 
> And let it handle the rs485.flags itself?
We will fix.
> 
> > +
> > +/**
> > + * cdns_uart_tx_empty -  Check whether TX is empty
> > + * @port: Handle to the uart port structure
> > + *
> > + * Return: TIOCSER_TEMT on success, 0 otherwise  */ static unsigned
> > +int cdns_uart_tx_empty(struct uart_port *port) {
> > +	unsigned int status;
> > +
> > +	status = readl(port->membase + CDNS_UART_SR) &
> > +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0; }
> > +
> >  /**
> >   * cdns_uart_handle_tx - Handle the bytes to be Txed.
> >   * @dev_id: Id of the UART port
> > @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct
> > notifier_block *nb,  static void cdns_uart_start_tx(struct uart_port
> > *port)  {
> >  	unsigned int status;
> > +	unsigned long time_out;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> >
> >  	if (uart_tx_stopped(port))
> >  		return;
> > @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)
> >
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_ISR);
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		cdns_rs485_tx_setup(cdns_uart);
> > +		if (cdns_uart->port->rs485.delay_rts_before_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_before_send);
> 
> Would it not be better to start a timer here with a callback that enables the
> TXEMPTY interrupt? That would automatically call cdns_uart_handle_tx().
We will fix.
> 
> > +	}
> > +
> >  	cdns_uart_handle_tx(port);
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> > +		/* Wait for tx completion */
> > +		while ((cdns_uart_tx_empty(cdns_uart->port) !=
> TIOCSER_TEMT) &&
> > +		       time_before(jiffies, time_out))
> > +			cpu_relax();
> > +
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		/*
> > +		 * Default Rx should be setup, because RX signaling path
> > +		 * need to enable to receive data.
> > +		 */
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> 
> I think this should be done from the TXEMPTY interrupt. And again schedule a
> timer to drop the DE line. You really can do this without using mdelay().
We will get TXEMPTY interrupt multiple times for large data, so doing this from
TXEMPTY interrupt will call this logic multiple times, but this logic need be to called once.
> 
> > +
> >  	/* Enable the TX Empty interrupt */
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_IER);  } @@
> > -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)
> >  static void cdns_uart_stop_tx(struct uart_port *port)  {
> >  	unsigned int regval;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> > +
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> 
> Again, start a timer and wait for completion?
We will fix.
> 
> >  	regval = readl(port->membase + CDNS_UART_CR);
> >  	regval |= CDNS_UART_CR_TX_DIS;
> > @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port
> > *port)
> >  	writel(regval, port->membase + CDNS_UART_CR);
> >  }
> >
> > -/**
> > - * cdns_uart_tx_empty -  Check whether TX is empty
> > - * @port: Handle to the uart port structure
> > - *
> > - * Return: TIOCSER_TEMT on success, 0 otherwise
> > - */
> > -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> > -{
> > -	unsigned int status;
> > -
> > -	status = readl(port->membase + CDNS_UART_SR) &
> > -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> > -}
> > -
> >  /**
> >   * cdns_uart_break_ctl - Based on the input ctl we have to start or
> > stop
> >   *			transmitting char breaks
> > @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port
> > *port)
> >  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
> >  		cpu_relax();
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +
> >  	/*
> >  	 * Clear the RX disable bit and then set the RX enable bit to enable
> >  	 * the receiver.
> > @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of,
> cdns_uart_of_match);
> >  /* Temporary variable for storing number of instances */
> >  static int instances;
> >
> > +/**
> > + * cdns_rs485_config - Called when an application calls TIOCSRS485
> > ioctl.
> > + * @port: Pointer to the uart_port structure
> > + * @termios: Pointer to the ktermios structure
> > + * @rs485: Pointer to the serial_rs485 structure
> > + *
> > + * Return: 0
> > + */
> > +static int cdns_rs485_config(struct uart_port *port, struct ktermios
> > *termios,
> > +			     struct serial_rs485 *rs485)
> > +{
> > +	if (rs485->flags & SER_RS485_ENABLED)
> > +		dev_dbg(port->dev, "Setting UART to RS485\n");
> > +
> 
> Shouldn't you force automatic RTS control off here?
> And also call cdns_rs485_rx_setup()
We will fix.
> 
> > +	return 0;
> > +}
> > +
> >  /**
> >   * cdns_uart_probe - Platform driver probe
> >   * @pdev: Pointer to the platform device structure
> > @@ -1463,6 +1587,7 @@ static int instances;
> >   */
> >  static int cdns_uart_probe(struct platform_device *pdev)
> >  {
> > +	u32 val;
> >  	int rc, id, irq;
> >  	struct uart_port *port;
> >  	struct resource *res;
> > @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct
> > platform_device *pdev)
> >  	port->private_data = cdns_uart_data;
> >  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY |
> CDNS_UART_IXR_RXTRIG
> > |
> >  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> > +	port->rs485_config = cdns_rs485_config;
> > +	port->rs485_supported = cdns_rs485_supported;
> >  	cdns_uart_data->port = port;
> >  	platform_set_drvdata(pdev, port);
> >
> > +	rc = uart_get_rs485_mode(port);
> > +	if (rc)
> > +		goto err_out_clk_notifier;
> > +
> > +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev,
> "rts",
> > +							GPIOD_OUT_LOW);
> > +	if (IS_ERR(cdns_uart_data->gpiod)) {
> > +		rc = PTR_ERR(cdns_uart_data->gpiod);
> > +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional
> failed\n");
> > +		goto err_out_clk_notifier;
> > +	}
> > +
> >  	pm_runtime_use_autosuspend(&pdev->dev);
> >  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> > UART_AUTOSUSPEND_TIMEOUT);
> >  	pm_runtime_set_active(&pdev->dev);
> > @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct
> > platform_device *pdev)
> >  	cdns_uart_data->cts_override =
> > of_property_read_bool(pdev->dev.of_node,
> >  							     "cts-override");
> >
> > +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (!cdns_uart_data->gpiod) {
> > +			val = readl(cdns_uart_data->port->membase
> > +				    + CDNS_UART_MODEMCR);
> > +			val |= CDNS_UART_MODEMCR_RTS;
> > +			writel(val, cdns_uart_data->port->membase
> > +			       + CDNS_UART_MODEMCR);
> > +		}
> > +	}
> 
> Simply call cdns_rs485_rx_setup() ?
We will fix.
> 
> > +
> >  	instances++;
> >
> >  	return 0;
> > @@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device
> > *pdev)
> >  	pm_runtime_disable(&pdev->dev);
> >  	pm_runtime_set_suspended(&pdev->dev);
> >  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> > +err_out_clk_notifier:
> >  #ifdef CONFIG_COMMON_CLK
> >  	clk_notifier_unregister(cdns_uart_data->uartclk,
> >  			&cdns_uart_data->clk_rate_change_nb);
> 
> Please also modify cdns_uart_[s|g]et_mctrl() so they support rts-gpios.
We will fix.
> 

Thanks,
Manikanta.

RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Guntupalli, Manikanta]

Hi,

> -----Original Message-----
> From: m.brock@vanmierlo.com <m.brock@vanmierlo.com>
> Sent: Saturday, November 4, 2023 9:17 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
> Cc: git (AMD-Xilinx) <git@amd.com>; Simek, Michal
> <michal.simek@amd.com>; gregkh@linuxfoundation.org;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> conor+dt@kernel.org; linux-serial@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> jirislaby@kernel.org; linux-arm-kernel@lists.infradead.org; Pandey, Radhey
> Shyam <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> <srinivas.goud@amd.com>; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; manion05gk@gmail.com
> Subject: Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps
> driver
> 
> Manikanta Guntupalli wrote on 2023-10-24 16:48:
> > @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-
> 255");
> >   * @clk_rate_change_nb:	Notifier block for clock changes
> >   * @quirks:		Flags for RXBS support.
> >   * @cts_override:	Modem control state override
> > + * @gpiod:		Pointer to the gpio descriptor
> 
> Change gpiod to gpiod_rts maybe?
> Later someone might want to use a gpio for cts/dtr/dsr/dcd/ri as well.
We will fix.
> 
> >   */
> >  struct cdns_uart {
> >  	struct uart_port	*port;
> > @@ -203,10 +207,19 @@ struct cdns_uart {
> >  	struct notifier_block	clk_rate_change_nb;
> >  	u32			quirks;
> >  	bool cts_override;
> > +	struct gpio_desc	*gpiod;
> >  };
> >  struct cdns_platform_data {
> >  	u32 quirks;
> >  };
> > +
> > +struct serial_rs485 cdns_rs485_supported = {
> > +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> > +		 SER_RS485_RTS_AFTER_SEND,
> > +	.delay_rts_before_send = 1,
> > +	.delay_rts_after_send = 1,
> > +};
> > +
> >  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
> >  		clk_rate_change_nb)
> >
> > @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id,
> > unsigned int isrstatus)
> >  	tty_flip_buffer_push(&port->state->port);
> >  }
> >
> > +/**
> > + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_high(struct cdns_uart
> > *cdns_uart)
> > +{
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 1);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val &= ~CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_low(struct cdns_uart
> > *cdns_uart)
> > +{
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 0);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val |= CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_tx_setup - Tx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_tx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> > +
> > +/**
> > + * cdns_rs485_rx_setup - Rx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_rx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> 
> Why not simply create:
> void cdns_rs485_driver_enable(struct cdns_uart *cdns_uart, bool enable)
We will fix.
> 
> And let it handle the rs485.flags itself?
We will fix.
> 
> > +
> > +/**
> > + * cdns_uart_tx_empty -  Check whether TX is empty
> > + * @port: Handle to the uart port structure
> > + *
> > + * Return: TIOCSER_TEMT on success, 0 otherwise  */ static unsigned
> > +int cdns_uart_tx_empty(struct uart_port *port) {
> > +	unsigned int status;
> > +
> > +	status = readl(port->membase + CDNS_UART_SR) &
> > +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0; }
> > +
> >  /**
> >   * cdns_uart_handle_tx - Handle the bytes to be Txed.
> >   * @dev_id: Id of the UART port
> > @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct
> > notifier_block *nb,  static void cdns_uart_start_tx(struct uart_port
> > *port)  {
> >  	unsigned int status;
> > +	unsigned long time_out;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> >
> >  	if (uart_tx_stopped(port))
> >  		return;
> > @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)
> >
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_ISR);
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		cdns_rs485_tx_setup(cdns_uart);
> > +		if (cdns_uart->port->rs485.delay_rts_before_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_before_send);
> 
> Would it not be better to start a timer here with a callback that enables the
> TXEMPTY interrupt? That would automatically call cdns_uart_handle_tx().
We will fix.
> 
> > +	}
> > +
> >  	cdns_uart_handle_tx(port);
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> > +		/* Wait for tx completion */
> > +		while ((cdns_uart_tx_empty(cdns_uart->port) !=
> TIOCSER_TEMT) &&
> > +		       time_before(jiffies, time_out))
> > +			cpu_relax();
> > +
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		/*
> > +		 * Default Rx should be setup, because RX signaling path
> > +		 * need to enable to receive data.
> > +		 */
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> 
> I think this should be done from the TXEMPTY interrupt. And again schedule a
> timer to drop the DE line. You really can do this without using mdelay().
We will get TXEMPTY interrupt multiple times for large data, so doing this from
TXEMPTY interrupt will call this logic multiple times, but this logic need be to called once.
> 
> > +
> >  	/* Enable the TX Empty interrupt */
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_IER);  } @@
> > -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)
> >  static void cdns_uart_stop_tx(struct uart_port *port)  {
> >  	unsigned int regval;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> > +
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> 
> Again, start a timer and wait for completion?
We will fix.
> 
> >  	regval = readl(port->membase + CDNS_UART_CR);
> >  	regval |= CDNS_UART_CR_TX_DIS;
> > @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port
> > *port)
> >  	writel(regval, port->membase + CDNS_UART_CR);
> >  }
> >
> > -/**
> > - * cdns_uart_tx_empty -  Check whether TX is empty
> > - * @port: Handle to the uart port structure
> > - *
> > - * Return: TIOCSER_TEMT on success, 0 otherwise
> > - */
> > -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> > -{
> > -	unsigned int status;
> > -
> > -	status = readl(port->membase + CDNS_UART_SR) &
> > -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> > -}
> > -
> >  /**
> >   * cdns_uart_break_ctl - Based on the input ctl we have to start or
> > stop
> >   *			transmitting char breaks
> > @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port
> > *port)
> >  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
> >  		cpu_relax();
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +
> >  	/*
> >  	 * Clear the RX disable bit and then set the RX enable bit to enable
> >  	 * the receiver.
> > @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of,
> cdns_uart_of_match);
> >  /* Temporary variable for storing number of instances */
> >  static int instances;
> >
> > +/**
> > + * cdns_rs485_config - Called when an application calls TIOCSRS485
> > ioctl.
> > + * @port: Pointer to the uart_port structure
> > + * @termios: Pointer to the ktermios structure
> > + * @rs485: Pointer to the serial_rs485 structure
> > + *
> > + * Return: 0
> > + */
> > +static int cdns_rs485_config(struct uart_port *port, struct ktermios
> > *termios,
> > +			     struct serial_rs485 *rs485)
> > +{
> > +	if (rs485->flags & SER_RS485_ENABLED)
> > +		dev_dbg(port->dev, "Setting UART to RS485\n");
> > +
> 
> Shouldn't you force automatic RTS control off here?
> And also call cdns_rs485_rx_setup()
We will fix.
> 
> > +	return 0;
> > +}
> > +
> >  /**
> >   * cdns_uart_probe - Platform driver probe
> >   * @pdev: Pointer to the platform device structure
> > @@ -1463,6 +1587,7 @@ static int instances;
> >   */
> >  static int cdns_uart_probe(struct platform_device *pdev)
> >  {
> > +	u32 val;
> >  	int rc, id, irq;
> >  	struct uart_port *port;
> >  	struct resource *res;
> > @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct
> > platform_device *pdev)
> >  	port->private_data = cdns_uart_data;
> >  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY |
> CDNS_UART_IXR_RXTRIG
> > |
> >  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> > +	port->rs485_config = cdns_rs485_config;
> > +	port->rs485_supported = cdns_rs485_supported;
> >  	cdns_uart_data->port = port;
> >  	platform_set_drvdata(pdev, port);
> >
> > +	rc = uart_get_rs485_mode(port);
> > +	if (rc)
> > +		goto err_out_clk_notifier;
> > +
> > +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev,
> "rts",
> > +							GPIOD_OUT_LOW);
> > +	if (IS_ERR(cdns_uart_data->gpiod)) {
> > +		rc = PTR_ERR(cdns_uart_data->gpiod);
> > +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional
> failed\n");
> > +		goto err_out_clk_notifier;
> > +	}
> > +
> >  	pm_runtime_use_autosuspend(&pdev->dev);
> >  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> > UART_AUTOSUSPEND_TIMEOUT);
> >  	pm_runtime_set_active(&pdev->dev);
> > @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct
> > platform_device *pdev)
> >  	cdns_uart_data->cts_override =
> > of_property_read_bool(pdev->dev.of_node,
> >  							     "cts-override");
> >
> > +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (!cdns_uart_data->gpiod) {
> > +			val = readl(cdns_uart_data->port->membase
> > +				    + CDNS_UART_MODEMCR);
> > +			val |= CDNS_UART_MODEMCR_RTS;
> > +			writel(val, cdns_uart_data->port->membase
> > +			       + CDNS_UART_MODEMCR);
> > +		}
> > +	}
> 
> Simply call cdns_rs485_rx_setup() ?
We will fix.
> 
> > +
> >  	instances++;
> >
> >  	return 0;
> > @@ -1646,6 +1795,7 @@ static int cdns_uart_probe(struct platform_device
> > *pdev)
> >  	pm_runtime_disable(&pdev->dev);
> >  	pm_runtime_set_suspended(&pdev->dev);
> >  	pm_runtime_dont_use_autosuspend(&pdev->dev);
> > +err_out_clk_notifier:
> >  #ifdef CONFIG_COMMON_CLK
> >  	clk_notifier_unregister(cdns_uart_data->uartclk,
> >  			&cdns_uart_data->clk_rate_change_nb);
> 
> Please also modify cdns_uart_[s|g]et_mctrl() so they support rts-gpios.
We will fix.
> 

Thanks,
Manikanta.

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

Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Lino Sanfilippo]

Hi,

On 24.10.23 16:48, Manikanta Guntupalli wrote:
> Add rs485 support to uartps driver. Use either rts-gpios or RTS
> to control RS485 phy as driver or a receiver.
>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Modify optional gpio name to xlnx,phy-ctrl-gpios.
> Update commit description.
> Add support for RTS, delay_rts_before_send and delay_rts_after_send in RS485 mode.
> Changes for V3:
> Modify optional gpio name to rts-gpios.
> Update commit description.
> Move cdns_uart_tx_empty function to avoid prototype statement.
> Remove assignment of struct serial_rs485 to port->rs485 as
> serial core performs that.
> Switch to native RTS in non GPIO case.
> Handle rs485 during stop tx.
> Remove explicit calls to configure gpio direction and value,
> as devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW argument.
> Update implementation to support configuration of GPIO/RTS value
> based on user configuration of SER_RS485_RTS_ON_SEND and
> SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from handle_tx.
> ---
>  drivers/tty/serial/xilinx_uartps.c | 180 ++++++++++++++++++++++++++---
>  1 file changed, 165 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 9c13dac1d4d1..32229cf5c508 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -23,6 +23,9 @@
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/iopoll.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>
>  #define CDNS_UART_TTY_NAME	"ttyPS"
>  #define CDNS_UART_NAME		"xuartps"
> @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
>   * @clk_rate_change_nb:	Notifier block for clock changes
>   * @quirks:		Flags for RXBS support.
>   * @cts_override:	Modem control state override
> + * @gpiod:		Pointer to the gpio descriptor
>   */
>  struct cdns_uart {
>  	struct uart_port	*port;
> @@ -203,10 +207,19 @@ struct cdns_uart {
>  	struct notifier_block	clk_rate_change_nb;
>  	u32			quirks;
>  	bool cts_override;
> +	struct gpio_desc	*gpiod;
>  };
>  struct cdns_platform_data {
>  	u32 quirks;
>  };
> +
> +struct serial_rs485 cdns_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_before_send = 1,
> +	.delay_rts_after_send = 1,
> +};
> +
>  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
>  		clk_rate_change_nb)
>
> @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
>  	tty_flip_buffer_push(&port->state->port);
>  }
>
> +/**
> + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_high(struct cdns_uart *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 1);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val &= ~CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_low(struct cdns_uart *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 0);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val |= CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_tx_setup - Tx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_tx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_rs485_rx_setup - Rx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_rx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_uart_tx_empty -  Check whether TX is empty
> + * @port: Handle to the uart port structure
> + *
> + * Return: TIOCSER_TEMT on success, 0 otherwise
> + */
> +static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> +{
> +	unsigned int status;
> +
> +	status = readl(port->membase + CDNS_UART_SR) &
> +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> +}
> +
>  /**
>   * cdns_uart_handle_tx - Handle the bytes to be Txed.
>   * @dev_id: Id of the UART port
> @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
>  static void cdns_uart_start_tx(struct uart_port *port)
>  {
>  	unsigned int status;
> +	unsigned long time_out;
> +	struct cdns_uart *cdns_uart = port->private_data;
>
>  	if (uart_tx_stopped(port))
>  		return;
> @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port *port)
>
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
>
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		cdns_rs485_tx_setup(cdns_uart);
> +		if (cdns_uart->port->rs485.delay_rts_before_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_before_send);

So, this will be executed each time (including the rts_before_send delay) the core wants
to send data? This is not how it is supposed to work: The tx setup (and the
delay before send) has to be done once when transmission starts. Note that when sending
a bulk of data the core may call cdns_uart_start_tx() several times before
it eventually calls cdns_uart_stop_tx() to stop the transmission.


> +	}
> +
>  	cdns_uart_handle_tx(port);
>
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> +		/* Wait for tx completion */
> +		while ((cdns_uart_tx_empty(cdns_uart->port) != TIOCSER_TEMT) &&
> +		       time_before(jiffies, time_out))
> +			cpu_relax();
> +
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		/*
> +		 * Default Rx should be setup, because RX signaling path
> +		 * need to enable to receive data.
> +		 */
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}
> +
>  	/* Enable the TX Empty interrupt */
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
>  }
> @@ -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
>  static void cdns_uart_stop_tx(struct uart_port *port)
>  {
>  	unsigned int regval;
> +	struct cdns_uart *cdns_uart = port->private_data;
> +
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}
>
>  	regval = readl(port->membase + CDNS_UART_CR);
>  	regval |= CDNS_UART_CR_TX_DIS;
> @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port *port)
>  	writel(regval, port->membase + CDNS_UART_CR);
>  }
>
> -/**
> - * cdns_uart_tx_empty -  Check whether TX is empty
> - * @port: Handle to the uart port structure
> - *
> - * Return: TIOCSER_TEMT on success, 0 otherwise
> - */
> -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> -{
> -	unsigned int status;
> -
> -	status = readl(port->membase + CDNS_UART_SR) &
> -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> -}
> -
>  /**
>   * cdns_uart_break_ctl - Based on the input ctl we have to start or stop
>   *			transmitting char breaks
> @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port *port)
>  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
>  		cpu_relax();
>
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> +		cdns_rs485_rx_setup(cdns_uart);
> +
>  	/*
>  	 * Clear the RX disable bit and then set the RX enable bit to enable
>  	 * the receiver.
> @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
>  /* Temporary variable for storing number of instances */
>  static int instances;
>
> +/**
> + * cdns_rs485_config - Called when an application calls TIOCSRS485 ioctl.
> + * @port: Pointer to the uart_port structure
> + * @termios: Pointer to the ktermios structure
> + * @rs485: Pointer to the serial_rs485 structure
> + *
> + * Return: 0
> + */
> +static int cdns_rs485_config(struct uart_port *port, struct ktermios *termios,
> +			     struct serial_rs485 *rs485)
> +{
> +	if (rs485->flags & SER_RS485_ENABLED)
> +		dev_dbg(port->dev, "Setting UART to RS485\n");
> +
> +	return 0;
> +}

So what if userspace changes the RS485 configuration? When does it take effect?

> +
>  /**
>   * cdns_uart_probe - Platform driver probe
>   * @pdev: Pointer to the platform device structure
> @@ -1463,6 +1587,7 @@ static int instances;
>   */
>  static int cdns_uart_probe(struct platform_device *pdev)
>  {
> +	u32 val;
>  	int rc, id, irq;
>  	struct uart_port *port;
>  	struct resource *res;
> @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	port->private_data = cdns_uart_data;
>  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG |
>  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> +	port->rs485_config = cdns_rs485_config;
> +	port->rs485_supported = cdns_rs485_supported;
>  	cdns_uart_data->port = port;
>  	platform_set_drvdata(pdev, port);
>
> +	rc = uart_get_rs485_mode(port);
> +	if (rc)
> +		goto err_out_clk_notifier;
> +
> +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev, "rts",
> +							GPIOD_OUT_LOW);
> +	if (IS_ERR(cdns_uart_data->gpiod)) {
> +		rc = PTR_ERR(cdns_uart_data->gpiod);
> +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional failed\n");

Why bail out with an error if having cdns_uart_data->gpiod is only optional?

> +		goto err_out_clk_notifier;
> +	}
> +
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_set_active(&pdev->dev);
> @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
>  							     "cts-override");
>
> +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (!cdns_uart_data->gpiod) {
> +			val = readl(cdns_uart_data->port->membase
> +				    + CDNS_UART_MODEMCR);
> +			val |= CDNS_UART_MODEMCR_RTS;
> +			writel(val, cdns_uart_data->port->membase
> +			       + CDNS_UART_MODEMCR);
> +		}
> +	}

This covers the RTS_AFTER_SEND mode. What if SER_RS485_RTS_ON_SEND is configured instead
(as it is the default set by uart_get_rs485_mode())? What if cdns_uart_data->gpiod exists?
Why not simply call cdns_rs485_rx_setup() which covers all these cases?
Note that uart_add_one_port() will call into the serial core and eventually result in an
initial call to the ports rs485_config function (see uart_rs485_config()). So maybe put the
initial configuration into that function and remove the above code. However
in this case

cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
							     "cts-override");
should be called before uart_add_one_port().


Regards,
Lino

Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Lino Sanfilippo]

Hi,

On 24.10.23 16:48, Manikanta Guntupalli wrote:
> Add rs485 support to uartps driver. Use either rts-gpios or RTS
> to control RS485 phy as driver or a receiver.
>
> Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> ---
> Changes for V2:
> Modify optional gpio name to xlnx,phy-ctrl-gpios.
> Update commit description.
> Add support for RTS, delay_rts_before_send and delay_rts_after_send in RS485 mode.
> Changes for V3:
> Modify optional gpio name to rts-gpios.
> Update commit description.
> Move cdns_uart_tx_empty function to avoid prototype statement.
> Remove assignment of struct serial_rs485 to port->rs485 as
> serial core performs that.
> Switch to native RTS in non GPIO case.
> Handle rs485 during stop tx.
> Remove explicit calls to configure gpio direction and value,
> as devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW argument.
> Update implementation to support configuration of GPIO/RTS value
> based on user configuration of SER_RS485_RTS_ON_SEND and
> SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from handle_tx.
> ---
>  drivers/tty/serial/xilinx_uartps.c | 180 ++++++++++++++++++++++++++---
>  1 file changed, 165 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 9c13dac1d4d1..32229cf5c508 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -23,6 +23,9 @@
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/iopoll.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
>
>  #define CDNS_UART_TTY_NAME	"ttyPS"
>  #define CDNS_UART_NAME		"xuartps"
> @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
>   * @clk_rate_change_nb:	Notifier block for clock changes
>   * @quirks:		Flags for RXBS support.
>   * @cts_override:	Modem control state override
> + * @gpiod:		Pointer to the gpio descriptor
>   */
>  struct cdns_uart {
>  	struct uart_port	*port;
> @@ -203,10 +207,19 @@ struct cdns_uart {
>  	struct notifier_block	clk_rate_change_nb;
>  	u32			quirks;
>  	bool cts_override;
> +	struct gpio_desc	*gpiod;
>  };
>  struct cdns_platform_data {
>  	u32 quirks;
>  };
> +
> +struct serial_rs485 cdns_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> +		 SER_RS485_RTS_AFTER_SEND,
> +	.delay_rts_before_send = 1,
> +	.delay_rts_after_send = 1,
> +};
> +
>  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
>  		clk_rate_change_nb)
>
> @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus)
>  	tty_flip_buffer_push(&port->state->port);
>  }
>
> +/**
> + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_high(struct cdns_uart *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 1);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val &= ~CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_config_gpio_rts_low(struct cdns_uart *cdns_uart)
> +{
> +	u32 val;
> +
> +	if (cdns_uart->gpiod) {
> +		gpiod_set_value(cdns_uart->gpiod, 0);
> +	} else {
> +		val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +		val |= CDNS_UART_MODEMCR_RTS;
> +		writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR);
> +	}
> +}
> +
> +/**
> + * cdns_rs485_tx_setup - Tx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_tx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_rs485_rx_setup - Rx setup specific to rs485
> + * @cdns_uart: Handle to the cdns_uart
> + */
> +static void cdns_rs485_rx_setup(struct cdns_uart *cdns_uart)
> +{
> +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> +	else
> +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> +}
> +
> +/**
> + * cdns_uart_tx_empty -  Check whether TX is empty
> + * @port: Handle to the uart port structure
> + *
> + * Return: TIOCSER_TEMT on success, 0 otherwise
> + */
> +static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> +{
> +	unsigned int status;
> +
> +	status = readl(port->membase + CDNS_UART_SR) &
> +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> +}
> +
>  /**
>   * cdns_uart_handle_tx - Handle the bytes to be Txed.
>   * @dev_id: Id of the UART port
> @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb,
>  static void cdns_uart_start_tx(struct uart_port *port)
>  {
>  	unsigned int status;
> +	unsigned long time_out;
> +	struct cdns_uart *cdns_uart = port->private_data;
>
>  	if (uart_tx_stopped(port))
>  		return;
> @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port *port)
>
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR);
>
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		cdns_rs485_tx_setup(cdns_uart);
> +		if (cdns_uart->port->rs485.delay_rts_before_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_before_send);

So, this will be executed each time (including the rts_before_send delay) the core wants
to send data? This is not how it is supposed to work: The tx setup (and the
delay before send) has to be done once when transmission starts. Note that when sending
a bulk of data the core may call cdns_uart_start_tx() several times before
it eventually calls cdns_uart_stop_tx() to stop the transmission.


> +	}
> +
>  	cdns_uart_handle_tx(port);
>
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> +		/* Wait for tx completion */
> +		while ((cdns_uart_tx_empty(cdns_uart->port) != TIOCSER_TEMT) &&
> +		       time_before(jiffies, time_out))
> +			cpu_relax();
> +
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		/*
> +		 * Default Rx should be setup, because RX signaling path
> +		 * need to enable to receive data.
> +		 */
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}
> +
>  	/* Enable the TX Empty interrupt */
>  	writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER);
>  }
> @@ -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
>  static void cdns_uart_stop_tx(struct uart_port *port)
>  {
>  	unsigned int regval;
> +	struct cdns_uart *cdns_uart = port->private_data;
> +
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (cdns_uart->port->rs485.delay_rts_after_send)
> +			mdelay(cdns_uart->port->rs485.delay_rts_after_send);
> +
> +		cdns_rs485_rx_setup(cdns_uart);
> +	}
>
>  	regval = readl(port->membase + CDNS_UART_CR);
>  	regval |= CDNS_UART_CR_TX_DIS;
> @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port *port)
>  	writel(regval, port->membase + CDNS_UART_CR);
>  }
>
> -/**
> - * cdns_uart_tx_empty -  Check whether TX is empty
> - * @port: Handle to the uart port structure
> - *
> - * Return: TIOCSER_TEMT on success, 0 otherwise
> - */
> -static unsigned int cdns_uart_tx_empty(struct uart_port *port)
> -{
> -	unsigned int status;
> -
> -	status = readl(port->membase + CDNS_UART_SR) &
> -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> -}
> -
>  /**
>   * cdns_uart_break_ctl - Based on the input ctl we have to start or stop
>   *			transmitting char breaks
> @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port *port)
>  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
>  		cpu_relax();
>
> +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> +		cdns_rs485_rx_setup(cdns_uart);
> +
>  	/*
>  	 * Clear the RX disable bit and then set the RX enable bit to enable
>  	 * the receiver.
> @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of, cdns_uart_of_match);
>  /* Temporary variable for storing number of instances */
>  static int instances;
>
> +/**
> + * cdns_rs485_config - Called when an application calls TIOCSRS485 ioctl.
> + * @port: Pointer to the uart_port structure
> + * @termios: Pointer to the ktermios structure
> + * @rs485: Pointer to the serial_rs485 structure
> + *
> + * Return: 0
> + */
> +static int cdns_rs485_config(struct uart_port *port, struct ktermios *termios,
> +			     struct serial_rs485 *rs485)
> +{
> +	if (rs485->flags & SER_RS485_ENABLED)
> +		dev_dbg(port->dev, "Setting UART to RS485\n");
> +
> +	return 0;
> +}

So what if userspace changes the RS485 configuration? When does it take effect?

> +
>  /**
>   * cdns_uart_probe - Platform driver probe
>   * @pdev: Pointer to the platform device structure
> @@ -1463,6 +1587,7 @@ static int instances;
>   */
>  static int cdns_uart_probe(struct platform_device *pdev)
>  {
> +	u32 val;
>  	int rc, id, irq;
>  	struct uart_port *port;
>  	struct resource *res;
> @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	port->private_data = cdns_uart_data;
>  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY | CDNS_UART_IXR_RXTRIG |
>  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> +	port->rs485_config = cdns_rs485_config;
> +	port->rs485_supported = cdns_rs485_supported;
>  	cdns_uart_data->port = port;
>  	platform_set_drvdata(pdev, port);
>
> +	rc = uart_get_rs485_mode(port);
> +	if (rc)
> +		goto err_out_clk_notifier;
> +
> +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev, "rts",
> +							GPIOD_OUT_LOW);
> +	if (IS_ERR(cdns_uart_data->gpiod)) {
> +		rc = PTR_ERR(cdns_uart_data->gpiod);
> +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional failed\n");

Why bail out with an error if having cdns_uart_data->gpiod is only optional?

> +		goto err_out_clk_notifier;
> +	}
> +
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_set_active(&pdev->dev);
> @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  	cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
>  							     "cts-override");
>
> +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> +		if (!cdns_uart_data->gpiod) {
> +			val = readl(cdns_uart_data->port->membase
> +				    + CDNS_UART_MODEMCR);
> +			val |= CDNS_UART_MODEMCR_RTS;
> +			writel(val, cdns_uart_data->port->membase
> +			       + CDNS_UART_MODEMCR);
> +		}
> +	}

This covers the RTS_AFTER_SEND mode. What if SER_RS485_RTS_ON_SEND is configured instead
(as it is the default set by uart_get_rs485_mode())? What if cdns_uart_data->gpiod exists?
Why not simply call cdns_rs485_rx_setup() which covers all these cases?
Note that uart_add_one_port() will call into the serial core and eventually result in an
initial call to the ports rs485_config function (see uart_rs485_config()). So maybe put the
initial configuration into that function and remove the above code. However
in this case

cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
							     "cts-override");
should be called before uart_add_one_port().


Regards,
Lino

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

RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Guntupalli, Manikanta]

Hi,

> -----Original Message-----
> From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> Sent: Monday, November 13, 2023 1:08 AM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-
> Xilinx) <git@amd.com>; Simek, Michal <michal.simek@amd.com>;
> gregkh@linuxfoundation.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; linux-
> serial@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; jirislaby@kernel.org; linux-arm-
> kernel@lists.infradead.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud,
> Srinivas <srinivas.goud@amd.com>; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; manion05gk@gmail.com
> Subject: Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps
> driver
> 
> Hi,
> 
> On 24.10.23 16:48, Manikanta Guntupalli wrote:
> > Add rs485 support to uartps driver. Use either rts-gpios or RTS to
> > control RS485 phy as driver or a receiver.
> >
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> > ---
> > Changes for V2:
> > Modify optional gpio name to xlnx,phy-ctrl-gpios.
> > Update commit description.
> > Add support for RTS, delay_rts_before_send and delay_rts_after_send in
> RS485 mode.
> > Changes for V3:
> > Modify optional gpio name to rts-gpios.
> > Update commit description.
> > Move cdns_uart_tx_empty function to avoid prototype statement.
> > Remove assignment of struct serial_rs485 to port->rs485 as serial core
> > performs that.
> > Switch to native RTS in non GPIO case.
> > Handle rs485 during stop tx.
> > Remove explicit calls to configure gpio direction and value, as
> > devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW
> argument.
> > Update implementation to support configuration of GPIO/RTS value based
> > on user configuration of SER_RS485_RTS_ON_SEND and
> > SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from
> handle_tx.
> > ---
> >  drivers/tty/serial/xilinx_uartps.c | 180
> > ++++++++++++++++++++++++++---
> >  1 file changed, 165 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/tty/serial/xilinx_uartps.c
> > b/drivers/tty/serial/xilinx_uartps.c
> > index 9c13dac1d4d1..32229cf5c508 100644
> > --- a/drivers/tty/serial/xilinx_uartps.c
> > +++ b/drivers/tty/serial/xilinx_uartps.c
> > @@ -23,6 +23,9 @@
> >  #include <linux/module.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> >
> >  #define CDNS_UART_TTY_NAME	"ttyPS"
> >  #define CDNS_UART_NAME		"xuartps"
> > @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-
> 255");
> >   * @clk_rate_change_nb:	Notifier block for clock changes
> >   * @quirks:		Flags for RXBS support.
> >   * @cts_override:	Modem control state override
> > + * @gpiod:		Pointer to the gpio descriptor
> >   */
> >  struct cdns_uart {
> >  	struct uart_port	*port;
> > @@ -203,10 +207,19 @@ struct cdns_uart {
> >  	struct notifier_block	clk_rate_change_nb;
> >  	u32			quirks;
> >  	bool cts_override;
> > +	struct gpio_desc	*gpiod;
> >  };
> >  struct cdns_platform_data {
> >  	u32 quirks;
> >  };
> > +
> > +struct serial_rs485 cdns_rs485_supported = {
> > +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> > +		 SER_RS485_RTS_AFTER_SEND,
> > +	.delay_rts_before_send = 1,
> > +	.delay_rts_after_send = 1,
> > +};
> > +
> >  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
> >  		clk_rate_change_nb)
> >
> > @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id,
> unsigned int isrstatus)
> >  	tty_flip_buffer_push(&port->state->port);
> >  }
> >
> > +/**
> > + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_high(struct cdns_uart *cdns_uart) {
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 1);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val &= ~CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_low(struct cdns_uart *cdns_uart) {
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 0);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val |= CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_tx_setup - Tx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_tx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> > +
> > +/**
> > + * cdns_rs485_rx_setup - Rx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_rx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> > +
> > +/**
> > + * cdns_uart_tx_empty -  Check whether TX is empty
> > + * @port: Handle to the uart port structure
> > + *
> > + * Return: TIOCSER_TEMT on success, 0 otherwise  */ static unsigned
> > +int cdns_uart_tx_empty(struct uart_port *port) {
> > +	unsigned int status;
> > +
> > +	status = readl(port->membase + CDNS_UART_SR) &
> > +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0; }
> > +
> >  /**
> >   * cdns_uart_handle_tx - Handle the bytes to be Txed.
> >   * @dev_id: Id of the UART port
> > @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct
> > notifier_block *nb,  static void cdns_uart_start_tx(struct uart_port
> > *port)  {
> >  	unsigned int status;
> > +	unsigned long time_out;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> >
> >  	if (uart_tx_stopped(port))
> >  		return;
> > @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)
> >
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_ISR);
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		cdns_rs485_tx_setup(cdns_uart);
> > +		if (cdns_uart->port->rs485.delay_rts_before_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_before_send);
> 
> So, this will be executed each time (including the rts_before_send delay) the
> core wants to send data? This is not how it is supposed to work: The tx setup
> (and the delay before send) has to be done once when transmission starts.
> Note that when sending a bulk of data the core may call cdns_uart_start_tx()
> several times before it eventually calls cdns_uart_stop_tx() to stop the
> transmission.
We have tested bulk transfer (5000 bytes) and observed that cdns_uart_start_tx() is getting called only once. Can you please suggest how to reproduce the case where the core may call cdns_uart_start_tx() several times before it eventually calls cdns_uart_stop_tx().
> 
> 
> > +	}
> > +
> >  	cdns_uart_handle_tx(port);
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> > +		/* Wait for tx completion */
> > +		while ((cdns_uart_tx_empty(cdns_uart->port) !=
> TIOCSER_TEMT) &&
> > +		       time_before(jiffies, time_out))
> > +			cpu_relax();
> > +
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		/*
> > +		 * Default Rx should be setup, because RX signaling path
> > +		 * need to enable to receive data.
> > +		 */
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> > +
> >  	/* Enable the TX Empty interrupt */
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_IER);  } @@
> > -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)  static void cdns_uart_stop_tx(struct uart_port *port)  {
> >  	unsigned int regval;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> > +
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> >
> >  	regval = readl(port->membase + CDNS_UART_CR);
> >  	regval |= CDNS_UART_CR_TX_DIS;
> > @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port
> *port)
> >  	writel(regval, port->membase + CDNS_UART_CR);  }
> >
> > -/**
> > - * cdns_uart_tx_empty -  Check whether TX is empty
> > - * @port: Handle to the uart port structure
> > - *
> > - * Return: TIOCSER_TEMT on success, 0 otherwise
> > - */
> > -static unsigned int cdns_uart_tx_empty(struct uart_port *port) -{
> > -	unsigned int status;
> > -
> > -	status = readl(port->membase + CDNS_UART_SR) &
> > -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> > -}
> > -
> >  /**
> >   * cdns_uart_break_ctl - Based on the input ctl we have to start or stop
> >   *			transmitting char breaks
> > @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port *port)
> >  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
> >  		cpu_relax();
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +
> >  	/*
> >  	 * Clear the RX disable bit and then set the RX enable bit to enable
> >  	 * the receiver.
> > @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of,
> cdns_uart_of_match);
> >  /* Temporary variable for storing number of instances */  static int
> > instances;
> >
> > +/**
> > + * cdns_rs485_config - Called when an application calls TIOCSRS485 ioctl.
> > + * @port: Pointer to the uart_port structure
> > + * @termios: Pointer to the ktermios structure
> > + * @rs485: Pointer to the serial_rs485 structure
> > + *
> > + * Return: 0
> > + */
> > +static int cdns_rs485_config(struct uart_port *port, struct ktermios
> *termios,
> > +			     struct serial_rs485 *rs485)
> > +{
> > +	if (rs485->flags & SER_RS485_ENABLED)
> > +		dev_dbg(port->dev, "Setting UART to RS485\n");
> > +
> > +	return 0;
> > +}
> 
> So what if userspace changes the RS485 configuration? When does it take
> effect?
We will disable auto RTS control and call cdns_uart_stop_tx() which will perform both stop tx and cdns_rs485_rx_setup()
> 
> > +
> >  /**
> >   * cdns_uart_probe - Platform driver probe
> >   * @pdev: Pointer to the platform device structure @@ -1463,6 +1587,7
> > @@ static int instances;
> >   */
> >  static int cdns_uart_probe(struct platform_device *pdev)  {
> > +	u32 val;
> >  	int rc, id, irq;
> >  	struct uart_port *port;
> >  	struct resource *res;
> > @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct
> platform_device *pdev)
> >  	port->private_data = cdns_uart_data;
> >  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY |
> CDNS_UART_IXR_RXTRIG |
> >  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> > +	port->rs485_config = cdns_rs485_config;
> > +	port->rs485_supported = cdns_rs485_supported;
> >  	cdns_uart_data->port = port;
> >  	platform_set_drvdata(pdev, port);
> >
> > +	rc = uart_get_rs485_mode(port);
> > +	if (rc)
> > +		goto err_out_clk_notifier;
> > +
> > +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev,
> "rts",
> > +							GPIOD_OUT_LOW);
> > +	if (IS_ERR(cdns_uart_data->gpiod)) {
> > +		rc = PTR_ERR(cdns_uart_data->gpiod);
> > +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional
> failed\n");
> 
> Why bail out with an error if having cdns_uart_data->gpiod is only optional?
We are using devm_gpiod_get_optional(), it will return NULL when gpio not passed, so IS_ERR() check will be false and will not  bail out.
> 
> > +		goto err_out_clk_notifier;
> > +	}
> > +
> >  	pm_runtime_use_autosuspend(&pdev->dev);
> >  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> UART_AUTOSUSPEND_TIMEOUT);
> >  	pm_runtime_set_active(&pdev->dev);
> > @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct
> platform_device *pdev)
> >  	cdns_uart_data->cts_override = of_property_read_bool(pdev-
> >dev.of_node,
> >  							     "cts-override");
> >
> > +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (!cdns_uart_data->gpiod) {
> > +			val = readl(cdns_uart_data->port->membase
> > +				    + CDNS_UART_MODEMCR);
> > +			val |= CDNS_UART_MODEMCR_RTS;
> > +			writel(val, cdns_uart_data->port->membase
> > +			       + CDNS_UART_MODEMCR);
> > +		}
> > +	}
> 
> This covers the RTS_AFTER_SEND mode. What if SER_RS485_RTS_ON_SEND is
> configured instead (as it is the default set by uart_get_rs485_mode())? What if
> cdns_uart_data->gpiod exists?
> Why not simply call cdns_rs485_rx_setup() which covers all these cases?
> Note that uart_add_one_port() will call into the serial core and eventually
> result in an initial call to the ports rs485_config function (see
> uart_rs485_config()). So maybe put the initial configuration into that function
> and remove the above code. However in this case
> 
> cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
> 							     "cts-override");
> should be called before uart_add_one_port().
We will fix.

Thanks,
Manikanta.

RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Guntupalli, Manikanta]

Hi,

> -----Original Message-----
> From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> Sent: Monday, November 13, 2023 1:08 AM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>; git (AMD-
> Xilinx) <git@amd.com>; Simek, Michal <michal.simek@amd.com>;
> gregkh@linuxfoundation.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; linux-
> serial@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; jirislaby@kernel.org; linux-arm-
> kernel@lists.infradead.org
> Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Goud,
> Srinivas <srinivas.goud@amd.com>; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; manion05gk@gmail.com
> Subject: Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps
> driver
> 
> Hi,
> 
> On 24.10.23 16:48, Manikanta Guntupalli wrote:
> > Add rs485 support to uartps driver. Use either rts-gpios or RTS to
> > control RS485 phy as driver or a receiver.
> >
> > Signed-off-by: Manikanta Guntupalli <manikanta.guntupalli@amd.com>
> > ---
> > Changes for V2:
> > Modify optional gpio name to xlnx,phy-ctrl-gpios.
> > Update commit description.
> > Add support for RTS, delay_rts_before_send and delay_rts_after_send in
> RS485 mode.
> > Changes for V3:
> > Modify optional gpio name to rts-gpios.
> > Update commit description.
> > Move cdns_uart_tx_empty function to avoid prototype statement.
> > Remove assignment of struct serial_rs485 to port->rs485 as serial core
> > performs that.
> > Switch to native RTS in non GPIO case.
> > Handle rs485 during stop tx.
> > Remove explicit calls to configure gpio direction and value, as
> > devm_gpiod_get_optional performs that by using GPIOD_OUT_LOW
> argument.
> > Update implementation to support configuration of GPIO/RTS value based
> > on user configuration of SER_RS485_RTS_ON_SEND and
> > SER_RS485_RTS_AFTER_SEND. Move implementation to start_tx from
> handle_tx.
> > ---
> >  drivers/tty/serial/xilinx_uartps.c | 180
> > ++++++++++++++++++++++++++---
> >  1 file changed, 165 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/tty/serial/xilinx_uartps.c
> > b/drivers/tty/serial/xilinx_uartps.c
> > index 9c13dac1d4d1..32229cf5c508 100644
> > --- a/drivers/tty/serial/xilinx_uartps.c
> > +++ b/drivers/tty/serial/xilinx_uartps.c
> > @@ -23,6 +23,9 @@
> >  #include <linux/module.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/delay.h>
> >
> >  #define CDNS_UART_TTY_NAME	"ttyPS"
> >  #define CDNS_UART_NAME		"xuartps"
> > @@ -193,6 +196,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-
> 255");
> >   * @clk_rate_change_nb:	Notifier block for clock changes
> >   * @quirks:		Flags for RXBS support.
> >   * @cts_override:	Modem control state override
> > + * @gpiod:		Pointer to the gpio descriptor
> >   */
> >  struct cdns_uart {
> >  	struct uart_port	*port;
> > @@ -203,10 +207,19 @@ struct cdns_uart {
> >  	struct notifier_block	clk_rate_change_nb;
> >  	u32			quirks;
> >  	bool cts_override;
> > +	struct gpio_desc	*gpiod;
> >  };
> >  struct cdns_platform_data {
> >  	u32 quirks;
> >  };
> > +
> > +struct serial_rs485 cdns_rs485_supported = {
> > +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> > +		 SER_RS485_RTS_AFTER_SEND,
> > +	.delay_rts_before_send = 1,
> > +	.delay_rts_after_send = 1,
> > +};
> > +
> >  #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
> >  		clk_rate_change_nb)
> >
> > @@ -305,6 +318,79 @@ static void cdns_uart_handle_rx(void *dev_id,
> unsigned int isrstatus)
> >  	tty_flip_buffer_push(&port->state->port);
> >  }
> >
> > +/**
> > + * cdns_rs485_config_gpio_rts_high - Configure GPIO/RTS to high
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_high(struct cdns_uart *cdns_uart) {
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 1);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val &= ~CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_config_gpio_rts_low - Configure GPIO/RTS to low
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_config_gpio_rts_low(struct cdns_uart *cdns_uart) {
> > +	u32 val;
> > +
> > +	if (cdns_uart->gpiod) {
> > +		gpiod_set_value(cdns_uart->gpiod, 0);
> > +	} else {
> > +		val = readl(cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +		val |= CDNS_UART_MODEMCR_RTS;
> > +		writel(val, cdns_uart->port->membase +
> CDNS_UART_MODEMCR);
> > +	}
> > +}
> > +
> > +/**
> > + * cdns_rs485_tx_setup - Tx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_tx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> > +
> > +/**
> > + * cdns_rs485_rx_setup - Rx setup specific to rs485
> > + * @cdns_uart: Handle to the cdns_uart  */ static void
> > +cdns_rs485_rx_setup(struct cdns_uart *cdns_uart) {
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> > +		cdns_rs485_config_gpio_rts_high(cdns_uart);
> > +	else
> > +		cdns_rs485_config_gpio_rts_low(cdns_uart);
> > +}
> > +
> > +/**
> > + * cdns_uart_tx_empty -  Check whether TX is empty
> > + * @port: Handle to the uart port structure
> > + *
> > + * Return: TIOCSER_TEMT on success, 0 otherwise  */ static unsigned
> > +int cdns_uart_tx_empty(struct uart_port *port) {
> > +	unsigned int status;
> > +
> > +	status = readl(port->membase + CDNS_UART_SR) &
> > +		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > +	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0; }
> > +
> >  /**
> >   * cdns_uart_handle_tx - Handle the bytes to be Txed.
> >   * @dev_id: Id of the UART port
> > @@ -571,6 +657,8 @@ static int cdns_uart_clk_notifier_cb(struct
> > notifier_block *nb,  static void cdns_uart_start_tx(struct uart_port
> > *port)  {
> >  	unsigned int status;
> > +	unsigned long time_out;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> >
> >  	if (uart_tx_stopped(port))
> >  		return;
> > @@ -589,8 +677,31 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)
> >
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_ISR);
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		cdns_rs485_tx_setup(cdns_uart);
> > +		if (cdns_uart->port->rs485.delay_rts_before_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_before_send);
> 
> So, this will be executed each time (including the rts_before_send delay) the
> core wants to send data? This is not how it is supposed to work: The tx setup
> (and the delay before send) has to be done once when transmission starts.
> Note that when sending a bulk of data the core may call cdns_uart_start_tx()
> several times before it eventually calls cdns_uart_stop_tx() to stop the
> transmission.
We have tested bulk transfer (5000 bytes) and observed that cdns_uart_start_tx() is getting called only once. Can you please suggest how to reproduce the case where the core may call cdns_uart_start_tx() several times before it eventually calls cdns_uart_stop_tx().
> 
> 
> > +	}
> > +
> >  	cdns_uart_handle_tx(port);
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		time_out = jiffies + usecs_to_jiffies(TX_TIMEOUT);
> > +		/* Wait for tx completion */
> > +		while ((cdns_uart_tx_empty(cdns_uart->port) !=
> TIOCSER_TEMT) &&
> > +		       time_before(jiffies, time_out))
> > +			cpu_relax();
> > +
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		/*
> > +		 * Default Rx should be setup, because RX signaling path
> > +		 * need to enable to receive data.
> > +		 */
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> > +
> >  	/* Enable the TX Empty interrupt */
> >  	writel(CDNS_UART_IXR_TXEMPTY, port->membase +
> CDNS_UART_IER);  } @@
> > -602,6 +713,14 @@ static void cdns_uart_start_tx(struct uart_port
> > *port)  static void cdns_uart_stop_tx(struct uart_port *port)  {
> >  	unsigned int regval;
> > +	struct cdns_uart *cdns_uart = port->private_data;
> > +
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (cdns_uart->port->rs485.delay_rts_after_send)
> > +			mdelay(cdns_uart->port-
> >rs485.delay_rts_after_send);
> > +
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +	}
> >
> >  	regval = readl(port->membase + CDNS_UART_CR);
> >  	regval |= CDNS_UART_CR_TX_DIS;
> > @@ -626,21 +745,6 @@ static void cdns_uart_stop_rx(struct uart_port
> *port)
> >  	writel(regval, port->membase + CDNS_UART_CR);  }
> >
> > -/**
> > - * cdns_uart_tx_empty -  Check whether TX is empty
> > - * @port: Handle to the uart port structure
> > - *
> > - * Return: TIOCSER_TEMT on success, 0 otherwise
> > - */
> > -static unsigned int cdns_uart_tx_empty(struct uart_port *port) -{
> > -	unsigned int status;
> > -
> > -	status = readl(port->membase + CDNS_UART_SR) &
> > -		       (CDNS_UART_SR_TXEMPTY | CDNS_UART_SR_TACTIVE);
> > -	return (status == CDNS_UART_SR_TXEMPTY) ? TIOCSER_TEMT : 0;
> > -}
> > -
> >  /**
> >   * cdns_uart_break_ctl - Based on the input ctl we have to start or stop
> >   *			transmitting char breaks
> > @@ -829,6 +933,9 @@ static int cdns_uart_startup(struct uart_port *port)
> >  		(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST))
> >  		cpu_relax();
> >
> > +	if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED)
> > +		cdns_rs485_rx_setup(cdns_uart);
> > +
> >  	/*
> >  	 * Clear the RX disable bit and then set the RX enable bit to enable
> >  	 * the receiver.
> > @@ -1455,6 +1562,23 @@ MODULE_DEVICE_TABLE(of,
> cdns_uart_of_match);
> >  /* Temporary variable for storing number of instances */  static int
> > instances;
> >
> > +/**
> > + * cdns_rs485_config - Called when an application calls TIOCSRS485 ioctl.
> > + * @port: Pointer to the uart_port structure
> > + * @termios: Pointer to the ktermios structure
> > + * @rs485: Pointer to the serial_rs485 structure
> > + *
> > + * Return: 0
> > + */
> > +static int cdns_rs485_config(struct uart_port *port, struct ktermios
> *termios,
> > +			     struct serial_rs485 *rs485)
> > +{
> > +	if (rs485->flags & SER_RS485_ENABLED)
> > +		dev_dbg(port->dev, "Setting UART to RS485\n");
> > +
> > +	return 0;
> > +}
> 
> So what if userspace changes the RS485 configuration? When does it take
> effect?
We will disable auto RTS control and call cdns_uart_stop_tx() which will perform both stop tx and cdns_rs485_rx_setup()
> 
> > +
> >  /**
> >   * cdns_uart_probe - Platform driver probe
> >   * @pdev: Pointer to the platform device structure @@ -1463,6 +1587,7
> > @@ static int instances;
> >   */
> >  static int cdns_uart_probe(struct platform_device *pdev)  {
> > +	u32 val;
> >  	int rc, id, irq;
> >  	struct uart_port *port;
> >  	struct resource *res;
> > @@ -1597,9 +1722,23 @@ static int cdns_uart_probe(struct
> platform_device *pdev)
> >  	port->private_data = cdns_uart_data;
> >  	port->read_status_mask = CDNS_UART_IXR_TXEMPTY |
> CDNS_UART_IXR_RXTRIG |
> >  			CDNS_UART_IXR_OVERRUN | CDNS_UART_IXR_TOUT;
> > +	port->rs485_config = cdns_rs485_config;
> > +	port->rs485_supported = cdns_rs485_supported;
> >  	cdns_uart_data->port = port;
> >  	platform_set_drvdata(pdev, port);
> >
> > +	rc = uart_get_rs485_mode(port);
> > +	if (rc)
> > +		goto err_out_clk_notifier;
> > +
> > +	cdns_uart_data->gpiod = devm_gpiod_get_optional(&pdev->dev,
> "rts",
> > +							GPIOD_OUT_LOW);
> > +	if (IS_ERR(cdns_uart_data->gpiod)) {
> > +		rc = PTR_ERR(cdns_uart_data->gpiod);
> > +		dev_err(port->dev, "xuartps: devm_gpiod_get_optional
> failed\n");
> 
> Why bail out with an error if having cdns_uart_data->gpiod is only optional?
We are using devm_gpiod_get_optional(), it will return NULL when gpio not passed, so IS_ERR() check will be false and will not  bail out.
> 
> > +		goto err_out_clk_notifier;
> > +	}
> > +
> >  	pm_runtime_use_autosuspend(&pdev->dev);
> >  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> UART_AUTOSUSPEND_TIMEOUT);
> >  	pm_runtime_set_active(&pdev->dev);
> > @@ -1638,6 +1777,16 @@ static int cdns_uart_probe(struct
> platform_device *pdev)
> >  	cdns_uart_data->cts_override = of_property_read_bool(pdev-
> >dev.of_node,
> >  							     "cts-override");
> >
> > +	if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) {
> > +		if (!cdns_uart_data->gpiod) {
> > +			val = readl(cdns_uart_data->port->membase
> > +				    + CDNS_UART_MODEMCR);
> > +			val |= CDNS_UART_MODEMCR_RTS;
> > +			writel(val, cdns_uart_data->port->membase
> > +			       + CDNS_UART_MODEMCR);
> > +		}
> > +	}
> 
> This covers the RTS_AFTER_SEND mode. What if SER_RS485_RTS_ON_SEND is
> configured instead (as it is the default set by uart_get_rs485_mode())? What if
> cdns_uart_data->gpiod exists?
> Why not simply call cdns_rs485_rx_setup() which covers all these cases?
> Note that uart_add_one_port() will call into the serial core and eventually
> result in an initial call to the ports rs485_config function (see
> uart_rs485_config()). So maybe put the initial configuration into that function
> and remove the above code. However in this case
> 
> cdns_uart_data->cts_override = of_property_read_bool(pdev->dev.of_node,
> 							     "cts-override");
> should be called before uart_add_one_port().
We will fix.

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

Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Lino Sanfilippo]

Hi,

On 15.11.23 07:50, Guntupalli, Manikanta wrote:

>> From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>>
>> So, this will be executed each time (including the rts_before_send delay) the
>> core wants to send data? This is not how it is supposed to work: The tx setup
>> (and the delay before send) has to be done once when transmission starts.
>> Note that when sending a bulk of data the core may call cdns_uart_start_tx()
>> several times before it eventually calls cdns_uart_stop_tx() to stop the
>> transmission.
> We have tested bulk transfer (5000 bytes) and observed that cdns_uart_start_tx() is getting called only once. Can you please suggest how to reproduce the case where the core may call cdns_uart_start_tx() several times before it eventually calls cdns_uart_stop_tx().


Thats strange. Normally the uart_ports start_tx() function is called whenever there is new
data in the tty circular buffer (see uart_write()) and the writing process is suspended after
that (see n_tty_write() in case of N_TTY line discipline). The writing process is woken up via
uart_write_wakeup() called from the uart driver and then it writes new data into the circular
buffer which results in another call to the uart_ports start_tx(). There is no stop_tx() until
all data from the passed userspace buffer is written. But there is a start_tx for every new bulk
of data that is available in the circular buffer.
This is at least what I can observe in my test setup (using a PL011 UART with the amba driver). If
I write a test buffer of 9212 bytes to the tty device using one single write() I can see 10
consecutive calls of tx_start() before tx_stop() eventually is called. What does your test setup look like?

Regards,
Lino

Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver

[Lino Sanfilippo]

Hi,

On 15.11.23 07:50, Guntupalli, Manikanta wrote:

>> From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>>
>> So, this will be executed each time (including the rts_before_send delay) the
>> core wants to send data? This is not how it is supposed to work: The tx setup
>> (and the delay before send) has to be done once when transmission starts.
>> Note that when sending a bulk of data the core may call cdns_uart_start_tx()
>> several times before it eventually calls cdns_uart_stop_tx() to stop the
>> transmission.
> We have tested bulk transfer (5000 bytes) and observed that cdns_uart_start_tx() is getting called only once. Can you please suggest how to reproduce the case where the core may call cdns_uart_start_tx() several times before it eventually calls cdns_uart_stop_tx().


Thats strange. Normally the uart_ports start_tx() function is called whenever there is new
data in the tty circular buffer (see uart_write()) and the writing process is suspended after
that (see n_tty_write() in case of N_TTY line discipline). The writing process is woken up via
uart_write_wakeup() called from the uart driver and then it writes new data into the circular
buffer which results in another call to the uart_ports start_tx(). There is no stop_tx() until
all data from the passed userspace buffer is written. But there is a start_tx for every new bulk
of data that is available in the circular buffer.
This is at least what I can observe in my test setup (using a PL011 UART with the amba driver). If
I write a test buffer of 9212 bytes to the tty device using one single write() I can see 10
consecutive calls of tx_start() before tx_stop() eventually is called. What does your test setup look like?

Regards,
Lino

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