| Message ID | 20231218094415.2503672-4-manikanta.guntupalli@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add rs485 support to uartps driver | expand |
Manikanta Guntupalli wrote on 2023-12-18 10:44: > drivers/tty/serial/xilinx_uartps.c | 222 +++++++++++++++++++++++++++-- > 1 file changed, 213 insertions(+), 9 deletions(-) > > @@ -203,10 +209,22 @@ struct cdns_uart { > struct notifier_block clk_rate_change_nb; > u32 quirks; > bool cts_override; > + struct gpio_desc *gpiod_rts; > + bool rs485_tx_started; > + struct timer_list timer; start_tx_timer > + struct timer_list stop_tx_timer; struct hrtimer maybe? > }; > 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 +323,55 @@ static void cdns_uart_handle_rx(void *dev_id, > unsigned int isrstatus) > tty_flip_buffer_push(&port->state->port); > } > > +/** > + * cdns_rts_gpio_enable - Configure RTS/GPIO to high/low > + * @cdns_uart: Handle to the cdns_uart > + * @enable: Value to be set to RTS/GPIO > + */ > +static void cdns_rts_gpio_enable(struct cdns_uart *cdns_uart, bool > enable) > +{ > + u32 val; > + > + if (cdns_uart->gpiod_rts) { > + gpiod_set_value(cdns_uart->gpiod_rts, enable); > + } else { > + val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR); > + if (enable) > + val &= ~CDNS_UART_MODEMCR_RTS; > + else > + 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_rts_gpio_enable(cdns_uart, 1); > + else > + cdns_rts_gpio_enable(cdns_uart, 0); Maybe simply: bool enable = cdns_uart->port->rs485.flags & SER_RS485_RTS_ON_SEND; cdns_rts_gpio_enable(cdns_uart, enable); > + > + cdns_uart->rs485_tx_started = true; > +} > + > +/** > + * 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_rts_gpio_enable(cdns_uart, 1); > + else > + cdns_rts_gpio_enable(cdns_uart, 0); Same here > + > + cdns_uart->rs485_tx_started = false; > +} > + > /** > * cdns_uart_tx_empty - Check whether TX is empty > * @port: Handle to the uart port structure > @@ -579,6 +646,44 @@ static int cdns_uart_clk_notifier_cb(struct > notifier_block *nb, > } > #endif > > +/** > + * cdns_rs485_rx_callback - Timer rx callback handler for rs485. > + * @t: Handle to the timer list structure > + */ > +static void cdns_rs485_rx_callback(struct timer_list *t) > +{ > + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, timer); > + > + /* > + * Default Rx should be setup, because Rx signaling path > + * need to enable to receive data. > + */ > + cdns_rs485_rx_setup(cdns_uart); > +} > + > +/** > + * cdns_rs485_tx_callback - Timer tx callback handler for rs485. > + * @t: Handle to the timer list structure > + */ > +static void cdns_rs485_tx_callback(struct timer_list *t) > +{ > + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, timer); > + > + uart_port_lock(cdns_uart->port); > + cdns_uart_handle_tx(cdns_uart->port); > + > + /* Enable the TX Empty interrupt */ > + writel(CDNS_UART_IXR_TXEMPTY, cdns_uart->port->membase + > CDNS_UART_IER); > + uart_port_unlock(cdns_uart->port); > + > + if (uart_circ_empty(&cdns_uart->port->state->xmit) || > + uart_tx_stopped(cdns_uart->port)) { > + timer_setup(&cdns_uart->timer, cdns_rs485_rx_callback, 0); You really should not do this here. This belongs in cdns_uart_handle_tx() which is also called from the TXEMPTY handler. And make sure TXEMPTY is true and on top you also must account for the time it takes for the last character to leave the transmitter including the stopbit. See also em485 code in 8250_port.c: stop_delay = p->port.frame_time + DIV_ROUND_UP(p->port.frame_time, 7); > + mod_timer(&cdns_uart->timer, jiffies + > + msecs_to_jiffies(cdns_uart->port->rs485.delay_rts_after_send)); > + } Should you not stop the stop_tx_timer in case it is still running when a new transmission is requested? > +} > + > /** > * cdns_uart_start_tx - Start transmitting bytes > * @port: Handle to the uart port structure > @@ -586,6 +691,7 @@ static int cdns_uart_clk_notifier_cb(struct > notifier_block *nb, > static void cdns_uart_start_tx(struct uart_port *port) > { > unsigned int status; > + struct cdns_uart *cdns_uart = port->private_data; > > if (uart_tx_stopped(port)) > return; > @@ -604,10 +710,40 @@ static void cdns_uart_start_tx(struct uart_port > *port) > > writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR); > > - cdns_uart_handle_tx(port); > + if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) { > + if (!cdns_uart->rs485_tx_started) { > + timer_setup(&cdns_uart->timer, > + cdns_rs485_tx_callback, 0); On a single line > + cdns_rs485_tx_setup(cdns_uart); > + mod_timer(&cdns_uart->timer, jiffies + > + msecs_to_jiffies(port->rs485.delay_rts_before_send)); > + } else { > + if (!timer_pending(&cdns_uart->timer)) > + mod_timer(&cdns_uart->timer, jiffies); > + } > + } else { > + cdns_uart_handle_tx(port); > > - /* Enable the TX Empty interrupt */ > - writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER); > + /* Enable the TX Empty interrupt */ > + writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER); > + } > +} > + > +/** > + * cdns_rs485_stop_tx_callback - Timer stop tx callback handler for > rs485. > + * @t: Handle to the timer list structure > + */ > +static void cdns_rs485_stop_tx_callback(struct timer_list *t) > +{ > + unsigned int regval; > + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, > stop_tx_timer); > + > + cdns_rs485_rx_setup(cdns_uart); > + > + regval = readl(cdns_uart->port->membase + CDNS_UART_CR); > + regval |= CDNS_UART_CR_TX_DIS; > + /* Disable the transmitter */ Why do you want to do this? > + writel(regval, cdns_uart->port->membase + CDNS_UART_CR); > } > > /** > @@ -617,11 +753,19 @@ 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; > > - regval = readl(port->membase + CDNS_UART_CR); > - regval |= CDNS_UART_CR_TX_DIS; > - /* Disable the transmitter */ > - writel(regval, port->membase + CDNS_UART_CR); > + if ((cdns_uart->port->rs485.flags & SER_RS485_ENABLED) && > + !timer_pending(&cdns_uart->stop_tx_timer) && > + cdns_uart->rs485_tx_started) { > + mod_timer(&cdns_uart->stop_tx_timer, jiffies + > + msecs_to_jiffies(cdns_uart->port->rs485.delay_rts_after_send)); Why try to adhere to the rts delay here? The original code doesn't seem to care if the fifo is still filled either. Or was it already broken? I did not yet find out exactly when this struct uart_ops .stop_tx is called. > + } else { > + regval = readl(port->membase + CDNS_UART_CR); > + regval |= CDNS_UART_CR_TX_DIS; > + /* Disable the transmitter */ > + writel(regval, port->membase + CDNS_UART_CR); > + } > } > > /** > @@ -829,6 +973,12 @@ 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) { > + timer_setup(&cdns_uart->stop_tx_timer, > + cdns_rs485_stop_tx_callback, 0); > + cdns_rs485_rx_setup(cdns_uart); > + } > + > /* > * Clear the RX disable bit and then set the RX enable bit to enable > * the receiver. > @@ -888,6 +1038,7 @@ static void cdns_uart_shutdown(struct uart_port > *port) > { > int status; > unsigned long flags; > + struct cdns_uart *cdns_uart = port->private_data; > > uart_port_lock_irqsave(port, &flags); > > @@ -903,6 +1054,11 @@ static void cdns_uart_shutdown(struct uart_port > *port) > uart_port_unlock_irqrestore(port, flags); > > free_irq(port->irq, port); > + > + if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) { > + del_timer_sync(&cdns_uart->timer); > + del_timer_sync(&cdns_uart->stop_tx_timer); > + } > } > > /** > @@ -1032,7 +1188,7 @@ static void cdns_uart_set_mctrl(struct uart_port > *port, unsigned int mctrl) > mode_reg &= ~CDNS_UART_MR_CHMODE_MASK; > > if (mctrl & TIOCM_RTS) > - val |= CDNS_UART_MODEMCR_RTS; > + cdns_rts_gpio_enable(cdns_uart_data, 1); First passing 1 here is wrong. It should be 0. Also there is no call with the opposite value here. But this call could modify the MODEMCR register however its result is immediately overwritten in the lines below with a wrong value in val. Keep as-is and maybe add the following instead: + if (cdns_uart->gpiod_rts) + gpiod_set_value(cdns_uart->gpiod_rts, !(mctrl & TIOCM_RTS)); > if (mctrl & TIOCM_DTR) > val |= CDNS_UART_MODEMCR_DTR; > if (mctrl & TIOCM_LOOP) > @@ -1455,6 +1611,37 @@ 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) > +{ > + u32 val; > + unsigned int ctrl_reg; > + > + if (rs485->flags & SER_RS485_ENABLED) { > + dev_dbg(port->dev, "Setting UART to RS485\n"); > + /* Make sure auto RTS is disabled */ > + val = readl(port->membase + CDNS_UART_MODEMCR); > + val &= ~CDNS_UART_MODEMCR_FCM; > + writel(val, port->membase + CDNS_UART_MODEMCR); > + /* Disable transmitter and make Rx setup*/ > + cdns_uart_stop_tx(port); > + } else { > + /* Disable the TX and RX */ > + ctrl_reg = readl(port->membase + CDNS_UART_CR); > + ctrl_reg |= CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS; > + writel(ctrl_reg, port->membase + CDNS_UART_CR); Why would you disable the transmitter and receiver here? > + } > + return 0; > +} > + > /** > * cdns_uart_probe - Platform driver probe > * @pdev: Pointer to the platform device structure > @@ -1597,9 +1784,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_rts = devm_gpiod_get_optional(&pdev->dev, > "rts", > + GPIOD_OUT_LOW); > + if (IS_ERR(cdns_uart_data->gpiod_rts)) { > + rc = PTR_ERR(cdns_uart_data->gpiod_rts); > + 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); > @@ -1618,6 +1819,8 @@ static int cdns_uart_probe(struct platform_device > *pdev) > console_port = port; > } > #endif > + if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) > + cdns_rs485_rx_setup(cdns_uart_data); > > rc = uart_add_one_port(&cdns_uart_uart_driver, port); > if (rc) { > @@ -1646,6 +1849,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); Kind Regards, Maarten Brock
Hi, > -----Original Message----- > From: m.brock@vanmierlo.com <m.brock@vanmierlo.com> > Sent: Monday, December 25, 2023 5:40 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 V7 3/3] tty: serial: uartps: Add rs485 support to uartps > driver > > Manikanta Guntupalli wrote on 2023-12-18 10:44: > > drivers/tty/serial/xilinx_uartps.c | 222 > > +++++++++++++++++++++++++++-- > > 1 file changed, 213 insertions(+), 9 deletions(-) > > > > @@ -203,10 +209,22 @@ struct cdns_uart { > > struct notifier_block clk_rate_change_nb; > > u32 quirks; > > bool cts_override; > > + struct gpio_desc *gpiod_rts; > > + bool rs485_tx_started; > > + struct timer_list timer; > > start_tx_timer We will fix. > > > + struct timer_list stop_tx_timer; > > struct hrtimer maybe? We will fix. > > > }; > > 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 +323,55 @@ static void cdns_uart_handle_rx(void *dev_id, > > unsigned int isrstatus) > > tty_flip_buffer_push(&port->state->port); > > } > > > > +/** > > + * cdns_rts_gpio_enable - Configure RTS/GPIO to high/low > > + * @cdns_uart: Handle to the cdns_uart > > + * @enable: Value to be set to RTS/GPIO */ static void > > +cdns_rts_gpio_enable(struct cdns_uart *cdns_uart, bool > > enable) > > +{ > > + u32 val; > > + > > + if (cdns_uart->gpiod_rts) { > > + gpiod_set_value(cdns_uart->gpiod_rts, enable); > > + } else { > > + val = readl(cdns_uart->port->membase + > CDNS_UART_MODEMCR); > > + if (enable) > > + val &= ~CDNS_UART_MODEMCR_RTS; > > + else > > + 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_rts_gpio_enable(cdns_uart, 1); > > + else > > + cdns_rts_gpio_enable(cdns_uart, 0); > > Maybe simply: > bool enable = cdns_uart->port->rs485.flags & > SER_RS485_RTS_ON_SEND; > cdns_rts_gpio_enable(cdns_uart, enable); We will fix. > > > + > > + cdns_uart->rs485_tx_started = true; > > +} > > + > > +/** > > + * 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_rts_gpio_enable(cdns_uart, 1); > > + else > > + cdns_rts_gpio_enable(cdns_uart, 0); > > Same here We will fix. > > > + > > + cdns_uart->rs485_tx_started = false; } > > + > > /** > > * cdns_uart_tx_empty - Check whether TX is empty > > * @port: Handle to the uart port structure @@ -579,6 +646,44 @@ > > static int cdns_uart_clk_notifier_cb(struct notifier_block *nb, } > > #endif > > > > +/** > > + * cdns_rs485_rx_callback - Timer rx callback handler for rs485. > > + * @t: Handle to the timer list structure */ static void > > +cdns_rs485_rx_callback(struct timer_list *t) { > > + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, timer); > > + > > + /* > > + * Default Rx should be setup, because Rx signaling path > > + * need to enable to receive data. > > + */ > > + cdns_rs485_rx_setup(cdns_uart); > > +} > > + > > +/** > > + * cdns_rs485_tx_callback - Timer tx callback handler for rs485. > > + * @t: Handle to the timer list structure */ static void > > +cdns_rs485_tx_callback(struct timer_list *t) { > > + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, timer); > > + > > + uart_port_lock(cdns_uart->port); > > + cdns_uart_handle_tx(cdns_uart->port); > > + > > + /* Enable the TX Empty interrupt */ > > + writel(CDNS_UART_IXR_TXEMPTY, cdns_uart->port->membase + > > CDNS_UART_IER); > > + uart_port_unlock(cdns_uart->port); > > + > > + if (uart_circ_empty(&cdns_uart->port->state->xmit) || > > + uart_tx_stopped(cdns_uart->port)) { > > + timer_setup(&cdns_uart->timer, cdns_rs485_rx_callback, 0); > > You really should not do this here. This belongs in > cdns_uart_handle_tx() which > is also called from the TXEMPTY handler. And make sure TXEMPTY is true and > on top you also must account for the time it takes for the last character to > leave the transmitter including the stopbit. > > See also em485 code in 8250_port.c: > stop_delay = p->port.frame_time + DIV_ROUND_UP(p- > >port.frame_time, 7); > > > + mod_timer(&cdns_uart->timer, jiffies + > > + msecs_to_jiffies(cdns_uart->port- > >rs485.delay_rts_after_send)); > > + } We will fix. > > Should you not stop the stop_tx_timer in case it is still running when a new > transmission is requested? > We will fix. > > +} > > + > > /** > > * cdns_uart_start_tx - Start transmitting bytes > > * @port: Handle to the uart port structure @@ -586,6 +691,7 @@ > > static int cdns_uart_clk_notifier_cb(struct notifier_block *nb, > > static void cdns_uart_start_tx(struct uart_port *port) { > > unsigned int status; > > + struct cdns_uart *cdns_uart = port->private_data; > > > > if (uart_tx_stopped(port)) > > return; > > @@ -604,10 +710,40 @@ static void cdns_uart_start_tx(struct uart_port > > *port) > > > > writel(CDNS_UART_IXR_TXEMPTY, port->membase + > CDNS_UART_ISR); > > > > - cdns_uart_handle_tx(port); > > + if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) { > > + if (!cdns_uart->rs485_tx_started) { > > + timer_setup(&cdns_uart->timer, > > + cdns_rs485_tx_callback, 0); > > On a single line We will fix. > > > + cdns_rs485_tx_setup(cdns_uart); > > + mod_timer(&cdns_uart->timer, jiffies + > > + msecs_to_jiffies(port- > >rs485.delay_rts_before_send)); > > + } else { > > + if (!timer_pending(&cdns_uart->timer)) > > + mod_timer(&cdns_uart->timer, jiffies); > > + } > > + } else { > > + cdns_uart_handle_tx(port); > > > > - /* Enable the TX Empty interrupt */ > > - writel(CDNS_UART_IXR_TXEMPTY, port->membase + > CDNS_UART_IER); > > + /* Enable the TX Empty interrupt */ > > + writel(CDNS_UART_IXR_TXEMPTY, port->membase + > CDNS_UART_IER); > > + } > > +} > > + > > +/** > > + * cdns_rs485_stop_tx_callback - Timer stop tx callback handler for > > rs485. > > + * @t: Handle to the timer list structure */ static void > > +cdns_rs485_stop_tx_callback(struct timer_list *t) { > > + unsigned int regval; > > + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, > > stop_tx_timer); > > + > > + cdns_rs485_rx_setup(cdns_uart); > > + > > + regval = readl(cdns_uart->port->membase + CDNS_UART_CR); > > + regval |= CDNS_UART_CR_TX_DIS; > > + /* Disable the transmitter */ > > Why do you want to do this? To disable Tx when cdns_uart_stop_tx() called in Rs485 case and this part of code runs in separate thread in RS485 case. > > > + writel(regval, cdns_uart->port->membase + CDNS_UART_CR); > > } > > > > /** > > @@ -617,11 +753,19 @@ 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; > > > > - regval = readl(port->membase + CDNS_UART_CR); > > - regval |= CDNS_UART_CR_TX_DIS; > > - /* Disable the transmitter */ > > - writel(regval, port->membase + CDNS_UART_CR); > > + if ((cdns_uart->port->rs485.flags & SER_RS485_ENABLED) && > > + !timer_pending(&cdns_uart->stop_tx_timer) && > > + cdns_uart->rs485_tx_started) { > > + mod_timer(&cdns_uart->stop_tx_timer, jiffies + > > + msecs_to_jiffies(cdns_uart->port- > >rs485.delay_rts_after_send)); > > Why try to adhere to the rts delay here? The original code doesn't seem > to care > if the fifo is still filled either. Or was it already broken? > > I did not yet find out exactly when this struct uart_ops .stop_tx is > called. As per kernel documentation, " the tty layer indicating we want to stop transmission due to an XOFF character." https://docs.kernel.org/driver-api/serial/driver.html > > > + } else { > > + regval = readl(port->membase + CDNS_UART_CR); > > + regval |= CDNS_UART_CR_TX_DIS; > > + /* Disable the transmitter */ > > + writel(regval, port->membase + CDNS_UART_CR); > > + } > > } > > > > /** > > @@ -829,6 +973,12 @@ 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) { > > + timer_setup(&cdns_uart->stop_tx_timer, > > + cdns_rs485_stop_tx_callback, 0); > > + cdns_rs485_rx_setup(cdns_uart); > > + } > > + > > /* > > * Clear the RX disable bit and then set the RX enable bit to enable > > * the receiver. > > @@ -888,6 +1038,7 @@ static void cdns_uart_shutdown(struct uart_port > > *port) > > { > > int status; > > unsigned long flags; > > + struct cdns_uart *cdns_uart = port->private_data; > > > > uart_port_lock_irqsave(port, &flags); > > > > @@ -903,6 +1054,11 @@ static void cdns_uart_shutdown(struct uart_port > > *port) > > uart_port_unlock_irqrestore(port, flags); > > > > free_irq(port->irq, port); > > + > > + if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) { > > + del_timer_sync(&cdns_uart->timer); > > + del_timer_sync(&cdns_uart->stop_tx_timer); > > + } > > } > > > > /** > > @@ -1032,7 +1188,7 @@ static void cdns_uart_set_mctrl(struct uart_port > > *port, unsigned int mctrl) > > mode_reg &= ~CDNS_UART_MR_CHMODE_MASK; > > > > if (mctrl & TIOCM_RTS) > > - val |= CDNS_UART_MODEMCR_RTS; > > + cdns_rts_gpio_enable(cdns_uart_data, 1); > > First passing 1 here is wrong. It should be 0. > Also there is no call with the opposite value here. > > But this call could modify the MODEMCR register however its result is > immediately overwritten in the lines below with a wrong value in val. > Keep as-is and maybe add the following instead: > > + if (cdns_uart->gpiod_rts) > + gpiod_set_value(cdns_uart->gpiod_rts, !(mctrl & > TIOCM_RTS)); GPIO is active high, so need to pass 1 for gpio case and to make native rts line high in native rts case need to write 0 to rts bit of MODEMCR that we are handling in cdns_rts_gpio_enable() as below: if (cdns_uart->gpiod_rts) { gpiod_set_value(cdns_uart->gpiod_rts, enable); } else { val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR); if (enable) val &= ~CDNS_UART_MODEMCR_RTS; else val |= CDNS_UART_MODEMCR_RTS; writel(val, cdns_uart->port->membase + CDNS_UART_MODEMCR); } > > > if (mctrl & TIOCM_DTR) > > val |= CDNS_UART_MODEMCR_DTR; > > if (mctrl & TIOCM_LOOP) > > @@ -1455,6 +1611,37 @@ 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) > > +{ > > + u32 val; > > + unsigned int ctrl_reg; > > + > > + if (rs485->flags & SER_RS485_ENABLED) { > > + dev_dbg(port->dev, "Setting UART to RS485\n"); > > + /* Make sure auto RTS is disabled */ > > + val = readl(port->membase + CDNS_UART_MODEMCR); > > + val &= ~CDNS_UART_MODEMCR_FCM; > > + writel(val, port->membase + CDNS_UART_MODEMCR); > > + /* Disable transmitter and make Rx setup*/ > > + cdns_uart_stop_tx(port); > > + } else { > > + /* Disable the TX and RX */ > > + ctrl_reg = readl(port->membase + CDNS_UART_CR); > > + ctrl_reg |= CDNS_UART_CR_TX_DIS | > CDNS_UART_CR_RX_DIS; > > + writel(ctrl_reg, port->membase + CDNS_UART_CR); > > Why would you disable the transmitter and receiver here? It was added as part of below review comment [1], please let me know what you recommend. " this function is expected to be able to also turn RS485 off." [1] https://lore.kernel.org/all/7919791e-f52f-eb35-ead-deea90cbe8@linux.intel.com/ > > > + } > > + return 0; > > +} > > + > > /** > > * cdns_uart_probe - Platform driver probe > > * @pdev: Pointer to the platform device structure > > @@ -1597,9 +1784,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_rts = devm_gpiod_get_optional(&pdev->dev, > > "rts", > > + GPIOD_OUT_LOW); > > + if (IS_ERR(cdns_uart_data->gpiod_rts)) { > > + rc = PTR_ERR(cdns_uart_data->gpiod_rts); > > + 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); > > @@ -1618,6 +1819,8 @@ static int cdns_uart_probe(struct platform_device > > *pdev) > > console_port = port; > > } > > #endif > > + if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) > > + cdns_rs485_rx_setup(cdns_uart_data); > > > > rc = uart_add_one_port(&cdns_uart_uart_driver, port); > > if (rc) { > > @@ -1646,6 +1849,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); > > Kind Regards, > Maarten Brock Thanks, Manikanta.
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c index aafcc2179e0e..04cc23deebdf 100644 --- a/drivers/tty/serial/xilinx_uartps.c +++ b/drivers/tty/serial/xilinx_uartps.c @@ -22,7 +22,9 @@ #include <linux/of.h> #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 +195,10 @@ 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_rts: Pointer to the gpio descriptor + * @rs485_tx_started: RS485 tx state + * @timer: Timer for tx and rx + * @stop_tx_timer: Timer for stop tx */ struct cdns_uart { struct uart_port *port; @@ -203,10 +209,22 @@ struct cdns_uart { struct notifier_block clk_rate_change_nb; u32 quirks; bool cts_override; + struct gpio_desc *gpiod_rts; + bool rs485_tx_started; + struct timer_list timer; + struct timer_list stop_tx_timer; }; 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 +323,55 @@ static void cdns_uart_handle_rx(void *dev_id, unsigned int isrstatus) tty_flip_buffer_push(&port->state->port); } +/** + * cdns_rts_gpio_enable - Configure RTS/GPIO to high/low + * @cdns_uart: Handle to the cdns_uart + * @enable: Value to be set to RTS/GPIO + */ +static void cdns_rts_gpio_enable(struct cdns_uart *cdns_uart, bool enable) +{ + u32 val; + + if (cdns_uart->gpiod_rts) { + gpiod_set_value(cdns_uart->gpiod_rts, enable); + } else { + val = readl(cdns_uart->port->membase + CDNS_UART_MODEMCR); + if (enable) + val &= ~CDNS_UART_MODEMCR_RTS; + else + 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_rts_gpio_enable(cdns_uart, 1); + else + cdns_rts_gpio_enable(cdns_uart, 0); + + cdns_uart->rs485_tx_started = true; +} + +/** + * 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_rts_gpio_enable(cdns_uart, 1); + else + cdns_rts_gpio_enable(cdns_uart, 0); + + cdns_uart->rs485_tx_started = false; +} + /** * cdns_uart_tx_empty - Check whether TX is empty * @port: Handle to the uart port structure @@ -579,6 +646,44 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb, } #endif +/** + * cdns_rs485_rx_callback - Timer rx callback handler for rs485. + * @t: Handle to the timer list structure + */ +static void cdns_rs485_rx_callback(struct timer_list *t) +{ + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, timer); + + /* + * Default Rx should be setup, because Rx signaling path + * need to enable to receive data. + */ + cdns_rs485_rx_setup(cdns_uart); +} + +/** + * cdns_rs485_tx_callback - Timer tx callback handler for rs485. + * @t: Handle to the timer list structure + */ +static void cdns_rs485_tx_callback(struct timer_list *t) +{ + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, timer); + + uart_port_lock(cdns_uart->port); + cdns_uart_handle_tx(cdns_uart->port); + + /* Enable the TX Empty interrupt */ + writel(CDNS_UART_IXR_TXEMPTY, cdns_uart->port->membase + CDNS_UART_IER); + uart_port_unlock(cdns_uart->port); + + if (uart_circ_empty(&cdns_uart->port->state->xmit) || + uart_tx_stopped(cdns_uart->port)) { + timer_setup(&cdns_uart->timer, cdns_rs485_rx_callback, 0); + mod_timer(&cdns_uart->timer, jiffies + + msecs_to_jiffies(cdns_uart->port->rs485.delay_rts_after_send)); + } +} + /** * cdns_uart_start_tx - Start transmitting bytes * @port: Handle to the uart port structure @@ -586,6 +691,7 @@ static int cdns_uart_clk_notifier_cb(struct notifier_block *nb, static void cdns_uart_start_tx(struct uart_port *port) { unsigned int status; + struct cdns_uart *cdns_uart = port->private_data; if (uart_tx_stopped(port)) return; @@ -604,10 +710,40 @@ static void cdns_uart_start_tx(struct uart_port *port) writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_ISR); - cdns_uart_handle_tx(port); + if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) { + if (!cdns_uart->rs485_tx_started) { + timer_setup(&cdns_uart->timer, + cdns_rs485_tx_callback, 0); + cdns_rs485_tx_setup(cdns_uart); + mod_timer(&cdns_uart->timer, jiffies + + msecs_to_jiffies(port->rs485.delay_rts_before_send)); + } else { + if (!timer_pending(&cdns_uart->timer)) + mod_timer(&cdns_uart->timer, jiffies); + } + } else { + cdns_uart_handle_tx(port); - /* Enable the TX Empty interrupt */ - writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER); + /* Enable the TX Empty interrupt */ + writel(CDNS_UART_IXR_TXEMPTY, port->membase + CDNS_UART_IER); + } +} + +/** + * cdns_rs485_stop_tx_callback - Timer stop tx callback handler for rs485. + * @t: Handle to the timer list structure + */ +static void cdns_rs485_stop_tx_callback(struct timer_list *t) +{ + unsigned int regval; + struct cdns_uart *cdns_uart = from_timer(cdns_uart, t, stop_tx_timer); + + cdns_rs485_rx_setup(cdns_uart); + + regval = readl(cdns_uart->port->membase + CDNS_UART_CR); + regval |= CDNS_UART_CR_TX_DIS; + /* Disable the transmitter */ + writel(regval, cdns_uart->port->membase + CDNS_UART_CR); } /** @@ -617,11 +753,19 @@ 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; - regval = readl(port->membase + CDNS_UART_CR); - regval |= CDNS_UART_CR_TX_DIS; - /* Disable the transmitter */ - writel(regval, port->membase + CDNS_UART_CR); + if ((cdns_uart->port->rs485.flags & SER_RS485_ENABLED) && + !timer_pending(&cdns_uart->stop_tx_timer) && + cdns_uart->rs485_tx_started) { + mod_timer(&cdns_uart->stop_tx_timer, jiffies + + msecs_to_jiffies(cdns_uart->port->rs485.delay_rts_after_send)); + } else { + regval = readl(port->membase + CDNS_UART_CR); + regval |= CDNS_UART_CR_TX_DIS; + /* Disable the transmitter */ + writel(regval, port->membase + CDNS_UART_CR); + } } /** @@ -829,6 +973,12 @@ 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) { + timer_setup(&cdns_uart->stop_tx_timer, + cdns_rs485_stop_tx_callback, 0); + cdns_rs485_rx_setup(cdns_uart); + } + /* * Clear the RX disable bit and then set the RX enable bit to enable * the receiver. @@ -888,6 +1038,7 @@ static void cdns_uart_shutdown(struct uart_port *port) { int status; unsigned long flags; + struct cdns_uart *cdns_uart = port->private_data; uart_port_lock_irqsave(port, &flags); @@ -903,6 +1054,11 @@ static void cdns_uart_shutdown(struct uart_port *port) uart_port_unlock_irqrestore(port, flags); free_irq(port->irq, port); + + if (cdns_uart->port->rs485.flags & SER_RS485_ENABLED) { + del_timer_sync(&cdns_uart->timer); + del_timer_sync(&cdns_uart->stop_tx_timer); + } } /** @@ -1032,7 +1188,7 @@ static void cdns_uart_set_mctrl(struct uart_port *port, unsigned int mctrl) mode_reg &= ~CDNS_UART_MR_CHMODE_MASK; if (mctrl & TIOCM_RTS) - val |= CDNS_UART_MODEMCR_RTS; + cdns_rts_gpio_enable(cdns_uart_data, 1); if (mctrl & TIOCM_DTR) val |= CDNS_UART_MODEMCR_DTR; if (mctrl & TIOCM_LOOP) @@ -1455,6 +1611,37 @@ 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) +{ + u32 val; + unsigned int ctrl_reg; + + if (rs485->flags & SER_RS485_ENABLED) { + dev_dbg(port->dev, "Setting UART to RS485\n"); + /* Make sure auto RTS is disabled */ + val = readl(port->membase + CDNS_UART_MODEMCR); + val &= ~CDNS_UART_MODEMCR_FCM; + writel(val, port->membase + CDNS_UART_MODEMCR); + /* Disable transmitter and make Rx setup*/ + cdns_uart_stop_tx(port); + } else { + /* Disable the TX and RX */ + ctrl_reg = readl(port->membase + CDNS_UART_CR); + ctrl_reg |= CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS; + writel(ctrl_reg, port->membase + CDNS_UART_CR); + } + return 0; +} + /** * cdns_uart_probe - Platform driver probe * @pdev: Pointer to the platform device structure @@ -1597,9 +1784,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_rts = devm_gpiod_get_optional(&pdev->dev, "rts", + GPIOD_OUT_LOW); + if (IS_ERR(cdns_uart_data->gpiod_rts)) { + rc = PTR_ERR(cdns_uart_data->gpiod_rts); + 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); @@ -1618,6 +1819,8 @@ static int cdns_uart_probe(struct platform_device *pdev) console_port = port; } #endif + if (cdns_uart_data->port->rs485.flags & SER_RS485_ENABLED) + cdns_rs485_rx_setup(cdns_uart_data); rc = uart_add_one_port(&cdns_uart_uart_driver, port); if (rc) { @@ -1646,6 +1849,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);
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. Changes for V4: Create separate patch for cdns_uart_tx_empty relocation. Call cdns_rs485_rx_setup() before uart_add_one_port() in probe. Update gpio descriptor name to gpiod_rts. Instead of cdns_rs485_config_gpio_rts_high() and cdns_rs485_config_gpio_rts_low() functions for RTS/GPIO value configuration implement cdns_rts_gpio_enable(). Disable auto rts and call cdns_uart_stop_tx() from cdns_rs485_config. Use timer instead of mdelay for delay_rts_before_send and delay_rts_after_send. Update cdns_uart_set_mctrl to support GPIO/RTS. Changes for V5: None. Changes for V6: Disable the TX and RX in cdns_rs485_config() when rs485 disabled. Hold lock for cdns_uart_handle_tx() in cdns_rs485_tx_callback(). Changes for V7: None. --- drivers/tty/serial/xilinx_uartps.c | 222 +++++++++++++++++++++++++++-- 1 file changed, 213 insertions(+), 9 deletions(-)