* [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
@ 2023-10-24 14:48 ` Manikanta Guntupalli
0 siblings, 0 replies; 22+ messages in thread
From: Manikanta Guntupalli @ 2023-10-24 14:48 UTC (permalink / raw)
To: git, michal.simek, gregkh, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel
Cc: radhey.shyam.pandey, srinivas.goud, shubhrajyoti.datta,
manion05gk, 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
2023-10-24 14:48 ` Manikanta Guntupalli
@ 2023-10-24 15:02 ` Ilpo Järvinen
-1 siblings, 0 replies; 22+ messages in thread
From: Ilpo Järvinen @ 2023-10-24 15:02 UTC (permalink / raw)
To: Manikanta Guntupalli
Cc: git, michal.simek, Greg Kroah-Hartman, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-serial, devicetree, LKML,
Jiri Slaby, linux-arm-kernel, radhey.shyam.pandey, srinivas.goud,
shubhrajyoti.datta, manion05gk
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
@ 2023-10-24 15:02 ` Ilpo Järvinen
0 siblings, 0 replies; 22+ messages in thread
From: Ilpo Järvinen @ 2023-10-24 15:02 UTC (permalink / raw)
To: Manikanta Guntupalli
Cc: git, michal.simek, Greg Kroah-Hartman, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-serial, devicetree, LKML,
Jiri Slaby, linux-arm-kernel, radhey.shyam.pandey, srinivas.goud,
shubhrajyoti.datta, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
2023-10-24 14:48 ` Manikanta Guntupalli
@ 2023-11-04 15:46 ` m.brock
-1 siblings, 0 replies; 22+ messages in thread
From: m.brock @ 2023-11-04 15:46 UTC (permalink / raw)
To: Manikanta Guntupalli
Cc: git, michal.simek, gregkh, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel, radhey.shyam.pandey, srinivas.goud,
shubhrajyoti.datta, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
@ 2023-11-04 15:46 ` m.brock
0 siblings, 0 replies; 22+ messages in thread
From: m.brock @ 2023-11-04 15:46 UTC (permalink / raw)
To: Manikanta Guntupalli
Cc: git, michal.simek, gregkh, robh+dt, krzysztof.kozlowski+dt,
conor+dt, linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel, radhey.shyam.pandey, srinivas.goud,
shubhrajyoti.datta, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
2023-11-04 15:46 ` m.brock
@ 2023-11-10 11:44 ` Guntupalli, Manikanta
-1 siblings, 0 replies; 22+ messages in thread
From: Guntupalli, Manikanta @ 2023-11-10 11:44 UTC (permalink / raw)
To: m.brock
Cc: git (AMD-Xilinx),
Simek, Michal, gregkh, robh+dt, krzysztof.kozlowski+dt, conor+dt,
linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel, Pandey, Radhey Shyam, Goud, Srinivas, Datta,
Shubhrajyoti, manion05gk
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
@ 2023-11-10 11:44 ` Guntupalli, Manikanta
0 siblings, 0 replies; 22+ messages in thread
From: Guntupalli, Manikanta @ 2023-11-10 11:44 UTC (permalink / raw)
To: m.brock
Cc: git (AMD-Xilinx),
Simek, Michal, gregkh, robh+dt, krzysztof.kozlowski+dt, conor+dt,
linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel, Pandey, Radhey Shyam, Goud, Srinivas, Datta,
Shubhrajyoti, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
2023-10-24 14:48 ` Manikanta Guntupalli
@ 2023-11-12 19:37 ` Lino Sanfilippo
-1 siblings, 0 replies; 22+ messages in thread
From: Lino Sanfilippo @ 2023-11-12 19:37 UTC (permalink / raw)
To: Manikanta Guntupalli, git, michal.simek, gregkh, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-serial, devicetree,
linux-kernel, jirislaby, linux-arm-kernel
Cc: radhey.shyam.pandey, srinivas.goud, shubhrajyoti.datta, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
@ 2023-11-12 19:37 ` Lino Sanfilippo
0 siblings, 0 replies; 22+ messages in thread
From: Lino Sanfilippo @ 2023-11-12 19:37 UTC (permalink / raw)
To: Manikanta Guntupalli, git, michal.simek, gregkh, robh+dt,
krzysztof.kozlowski+dt, conor+dt, linux-serial, devicetree,
linux-kernel, jirislaby, linux-arm-kernel
Cc: radhey.shyam.pandey, srinivas.goud, shubhrajyoti.datta, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
2023-11-12 19:37 ` Lino Sanfilippo
@ 2023-11-15 6:50 ` Guntupalli, Manikanta
-1 siblings, 0 replies; 22+ messages in thread
From: Guntupalli, Manikanta @ 2023-11-15 6:50 UTC (permalink / raw)
To: Lino Sanfilippo, git (AMD-Xilinx),
Simek, Michal, gregkh, robh+dt, krzysztof.kozlowski+dt, conor+dt,
linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel
Cc: Pandey, Radhey Shyam, Goud, Srinivas, Datta, Shubhrajyoti, manion05gk
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.
^ permalink raw reply [flat|nested] 22+ messages in thread* RE: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
@ 2023-11-15 6:50 ` Guntupalli, Manikanta
0 siblings, 0 replies; 22+ messages in thread
From: Guntupalli, Manikanta @ 2023-11-15 6:50 UTC (permalink / raw)
To: Lino Sanfilippo, git (AMD-Xilinx),
Simek, Michal, gregkh, robh+dt, krzysztof.kozlowski+dt, conor+dt,
linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel
Cc: Pandey, Radhey Shyam, Goud, Srinivas, Datta, Shubhrajyoti, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
2023-11-15 6:50 ` Guntupalli, Manikanta
@ 2023-11-18 19:35 ` Lino Sanfilippo
-1 siblings, 0 replies; 22+ messages in thread
From: Lino Sanfilippo @ 2023-11-18 19:35 UTC (permalink / raw)
To: Guntupalli, Manikanta, git (AMD-Xilinx),
Simek, Michal, gregkh, robh+dt, krzysztof.kozlowski+dt, conor+dt,
linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel
Cc: Pandey, Radhey Shyam, Goud, Srinivas, Datta, Shubhrajyoti, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V3 2/2] tty: serial: uartps: Add rs485 support to uartps driver
@ 2023-11-18 19:35 ` Lino Sanfilippo
0 siblings, 0 replies; 22+ messages in thread
From: Lino Sanfilippo @ 2023-11-18 19:35 UTC (permalink / raw)
To: Guntupalli, Manikanta, git (AMD-Xilinx),
Simek, Michal, gregkh, robh+dt, krzysztof.kozlowski+dt, conor+dt,
linux-serial, devicetree, linux-kernel, jirislaby,
linux-arm-kernel
Cc: Pandey, Radhey Shyam, Goud, Srinivas, Datta, Shubhrajyoti, manion05gk
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
^ permalink raw reply [flat|nested] 22+ messages in thread