* Re: [PATCH] serial: Deassert RS485 Transmit Enable on probe in driver-specific way
[not found] <b17842d9792762948f75d74205596d36bdd27f22.1663310999.git.lukas@wunner.de>
@ 2022-09-16 11:22 ` Matthias Schiffer
2022-09-16 12:27 ` Ilpo Järvinen
1 sibling, 0 replies; 3+ messages in thread
From: Matthias Schiffer @ 2022-09-16 11:22 UTC (permalink / raw)
To: Lukas Wunner
Cc: Jiri Slaby, Lino Sanfilippo, Ilpo Jarvinen, David Laight,
Maarten Brock, Jan Kiszka, Su Bao Cheng, baocheng_su, Chao Zeng,
Peter Hung, Joachim Eastwood, Daniel Golle,
<Codrin.Ciubotariu@microchip.com>,
Sherry Sun, Serge Semin, Ricardo Ribalda, Dario Binacchi,
Bich Hemon, Marek Vasut, Greg Kroah-Hartman, Roosen Henri,
linux-serial
On Fri, 2022-09-16 at 09:04 +0200, Lukas Wunner wrote:
> When a UART port is newly registered, uart_configure_port() seeks to
> deassert RS485 Transmit Enable by setting the RTS bit in port->mctrl.
> However a number of UART drivers interpret a set RTS bit as *assertion*
> instead of deassertion: Affected drivers include those using
> serial8250_em485_config() (except 8250_bcm2835aux.c) and some using
> mctrl_gpio (e.g. imx.c).
>
> Since the interpretation of the RTS bit is driver-specific, it is not
> suitable as a means to centrally deassert Transmit Enable in the serial
> core. Instead, the serial core must call on drivers to deassert it in
> their driver-specific way. One way to achieve that is to call
> ->rs485_config(). It implicitly deasserts Transmit Enable.
>
> So amend uart_configure_port() and uart_resume_port() to invoke
> uart_rs485_config(). That allows removing calls to uart_rs485_config()
> from drivers' ->probe() hooks and declaring the function static.
>
> Skip any invocation of ->set_mctrl() if RS485 is enabled. RS485 has no
> hardware flow control, so the modem control lines are irrelevant and
> need not be touched. When leaving RS485 mode, reset the modem control
> lines to the state stored in port->mctrl. That way, UARTs which are
> muxed between RS485 and RS232 transceivers drive the lines correctly
> when switched to RS232. (serial8250_do_startup() historically raises
> the OUT1 modem signal because otherwise interrupts are not signaled on
> ancient PC UARTs, but I believe that no longer applies to modern,
> RS485-capable UARTs and is thus safe to be skipped.)
>
> imx.c modifies port->mctrl whenever Transmit Enable is asserted and
> deasserted. Stop it from doing that so port->mctrl reflects the RS232
> line state.
>
> 8250_omap.c deasserts Transmit Enable on ->runtime_resume() by calling
> ->set_mctrl(). Because that is now a no-op in RS485 mode, amend the
> function to call serial8250_em485_stop_tx().
>
> fsl_lpuart.c retrieves and applies the RS485 device tree properties
> after registering the UART port. Because applying now happens on
> registration in uart_configure_port(), move retrieval of the properties
> ahead of uart_add_one_port().
>
> Fixes: d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart open")
> Reported-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Link: https://lore.kernel.org/all/20220329085050.311408-1-matthias.schiffer@ew.tq-group.com/
> Reported-by: Roosen Henri <Henri.Roosen@ginzinger.com>
> Link: https://lore.kernel.org/all/8f538a8903795f22f9acc94a9a31b03c9c4ccacb.camel@ginzinger.com/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.14+
I had to add one more fix to make RS485 work at all with 8250_omap on
Linux 6.0-rc5+ (submitted a few minutes ago, "serial: 8250: omap: Use
serial8250_em485_supported"), but the RTS polarity is correct now, so:
Tested-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
Thanks!
> ---
> Based on v6.0-rc3 + this dependency:
> https://lore.kernel.org/linux-serial/72fb646c1b0b11c989850c55f52f9ff343d1b2fa.1662884345.git.lukas@wunner.de/
>
> drivers/tty/serial/8250/8250_omap.c | 3 +++
> drivers/tty/serial/8250/8250_pci.c | 9 +--------
> drivers/tty/serial/8250/8250_port.c | 10 ++++++----
> drivers/tty/serial/fsl_lpuart.c | 10 ++++------
> drivers/tty/serial/imx.c | 8 ++------
> drivers/tty/serial/serial_core.c | 36 ++++++++++++++++++++----------------
> include/linux/serial_core.h | 1 -
> 7 files changed, 36 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index b43894e..2e67754 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -342,6 +342,9 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
> omap8250_update_mdr1(up, priv);
>
> up->port.ops->set_mctrl(&up->port, up->port.mctrl);
> +
> + if (up->port.rs485.flags & SER_RS485_ENABLED)
> + serial8250_em485_stop_tx(up);
> }
>
> /*
> diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
> index 6f66dc2..b260c55 100644
> --- a/drivers/tty/serial/8250/8250_pci.c
> +++ b/drivers/tty/serial/8250/8250_pci.c
> @@ -1627,7 +1627,6 @@ static int pci_fintek_init(struct pci_dev *dev)
> resource_size_t bar_data[3];
> u8 config_base;
> struct serial_private *priv = pci_get_drvdata(dev);
> - struct uart_8250_port *port;
>
> if (!(pci_resource_flags(dev, 5) & IORESOURCE_IO) ||
> !(pci_resource_flags(dev, 4) & IORESOURCE_IO) ||
> @@ -1674,13 +1673,7 @@ static int pci_fintek_init(struct pci_dev *dev)
>
> pci_write_config_byte(dev, config_base + 0x06, dev->irq);
>
> - if (priv) {
> - /* re-apply RS232/485 mode when
> - * pciserial_resume_ports()
> - */
> - port = serial8250_get_port(priv->line[i]);
> - uart_rs485_config(&port->port);
> - } else {
> + if (!priv) {
> /* First init without port data
> * force init to RS232 Mode
> */
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 907c5ff..a6f03b1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -600,7 +600,7 @@ void serial8250_rpm_put(struct uart_8250_port *p)
> static int serial8250_em485_init(struct uart_8250_port *p)
> {
> if (p->em485)
> - return 0;
> + goto deassert_rts;
>
> p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_ATOMIC);
> if (!p->em485)
> @@ -616,6 +616,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
> p->em485->active_timer = NULL;
> p->em485->tx_stopped = true;
>
> +deassert_rts:
> p->rs485_stop_tx(p);
>
> return 0;
> @@ -2047,6 +2048,10 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
>
> static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
> {
> +
> + if (port->rs485.flags & SER_RS485_ENABLED)
> + return;
> +
> if (port->set_mctrl)
> port->set_mctrl(port, mctrl);
> else
> @@ -3189,9 +3194,6 @@ static void serial8250_config_port(struct uart_port *port, int flags)
> if (flags & UART_CONFIG_TYPE)
> autoconfig(up);
>
> - if (port->rs485.flags & SER_RS485_ENABLED)
> - uart_rs485_config(port);
> -
> /* if access method is AU, it is a 16550 with a quirk */
> if (port->type == PORT_16550A && port->iotype == UPIO_AU)
> up->bugs |= UART_BUG_NOMSR;
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index e9d5b48..dfc73fe 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -2724,15 +2724,13 @@ static int lpuart_probe(struct platform_device *pdev)
> if (ret)
> goto failed_reset;
>
> - ret = uart_add_one_port(&lpuart_reg, &sport->port);
> - if (ret)
> - goto failed_attach_port;
> -
> ret = uart_get_rs485_mode(&sport->port);
> if (ret)
> goto failed_get_rs485;
>
> - uart_rs485_config(&sport->port);
> + ret = uart_add_one_port(&lpuart_reg, &sport->port);
> + if (ret)
> + goto failed_attach_port;
>
> ret = devm_request_irq(&pdev->dev, sport->port.irq, handler, 0,
> DRIVER_NAME, sport);
> @@ -2742,9 +2740,9 @@ static int lpuart_probe(struct platform_device *pdev)
> return 0;
>
> failed_irq_request:
> -failed_get_rs485:
> uart_remove_one_port(&lpuart_reg, &sport->port);
> failed_attach_port:
> +failed_get_rs485:
> failed_reset:
> lpuart_disable_clks(sport);
> return ret;
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 5875ee6..05b432d 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -380,8 +380,7 @@ static void imx_uart_rts_active(struct imx_port *sport, u32 *ucr2)
> {
> *ucr2 &= ~(UCR2_CTSC | UCR2_CTS);
>
> - sport->port.mctrl |= TIOCM_RTS;
> - mctrl_gpio_set(sport->gpios, sport->port.mctrl);
> + mctrl_gpio_set(sport->gpios, sport->port.mctrl | TIOCM_RTS);
> }
>
> /* called with port.lock taken and irqs caller dependent */
> @@ -390,8 +389,7 @@ static void imx_uart_rts_inactive(struct imx_port *sport, u32 *ucr2)
> *ucr2 &= ~UCR2_CTSC;
> *ucr2 |= UCR2_CTS;
>
> - sport->port.mctrl &= ~TIOCM_RTS;
> - mctrl_gpio_set(sport->gpios, sport->port.mctrl);
> + mctrl_gpio_set(sport->gpios, sport->port.mctrl & ~TIOCM_RTS);
> }
>
> static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
> @@ -2347,8 +2345,6 @@ static int imx_uart_probe(struct platform_device *pdev)
> dev_err(&pdev->dev,
> "low-active RTS not possible when receiver is off, enabling receiver\n");
>
> - uart_rs485_config(&sport->port);
> -
> /* Disable interrupts before requesting them */
> ucr1 = imx_uart_readl(sport, UCR1);
> ucr1 &= ~(UCR1_ADEN | UCR1_TRDYEN | UCR1_IDEN | UCR1_RRDYEN | UCR1_RTSDEN);
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index c711318..179ee19 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -158,15 +158,10 @@ static void uart_start(struct tty_struct *tty)
> unsigned long flags;
> unsigned int old;
>
> - if (port->rs485.flags & SER_RS485_ENABLED) {
> - set &= ~TIOCM_RTS;
> - clear &= ~TIOCM_RTS;
> - }
> -
> spin_lock_irqsave(&port->lock, flags);
> old = port->mctrl;
> port->mctrl = (old & ~clear) | set;
> - if (old != port->mctrl)
> + if (old != port->mctrl && !(port->rs485.flags & SER_RS485_ENABLED))
> port->ops->set_mctrl(port, port->mctrl);
> spin_unlock_irqrestore(&port->lock, flags);
> }
> @@ -1391,7 +1386,7 @@ static void uart_set_rs485_termination(struct uart_port *port,
> !!(rs485->flags & SER_RS485_TERMINATE_BUS));
> }
>
> -int uart_rs485_config(struct uart_port *port)
> +static int uart_rs485_config(struct uart_port *port)
> {
> struct serial_rs485 *rs485 = &port->rs485;
> int ret;
> @@ -1405,7 +1400,6 @@ int uart_rs485_config(struct uart_port *port)
>
> return ret;
> }
> -EXPORT_SYMBOL_GPL(uart_rs485_config);
>
> static int uart_get_rs485_config(struct uart_port *port,
> struct serial_rs485 __user *rs485)
> @@ -1444,8 +1438,13 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
>
> spin_lock_irqsave(&port->lock, flags);
> ret = port->rs485_config(port, &tty->termios, &rs485);
> - if (!ret)
> + if (!ret) {
> port->rs485 = rs485;
> +
> + /* Reset RTS and other mctrl lines when disabling RS485 */
> + if (!(rs485.flags & SER_RS485_ENABLED))
> + port->ops->set_mctrl(port, port->mctrl);
> + }
> spin_unlock_irqrestore(&port->lock, flags);
> if (ret)
> return ret;
> @@ -2352,7 +2351,8 @@ int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
>
> spin_lock_irq(&uport->lock);
> ops->stop_tx(uport);
> - ops->set_mctrl(uport, 0);
> + if (!(uport->rs485.flags & SER_RS485_ENABLED))
> + ops->set_mctrl(uport, 0);
> /* save mctrl so it can be restored on resume */
> mctrl = uport->mctrl;
> uport->mctrl = 0;
> @@ -2440,7 +2440,8 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
>
> uart_change_pm(state, UART_PM_STATE_ON);
> spin_lock_irq(&uport->lock);
> - ops->set_mctrl(uport, 0);
> + if (!(uport->rs485.flags & SER_RS485_ENABLED))
> + ops->set_mctrl(uport, 0);
> spin_unlock_irq(&uport->lock);
> if (console_suspend_enabled || !uart_console(uport)) {
> /* Protected by port mutex for now */
> @@ -2451,7 +2452,10 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
> if (tty)
> uart_change_speed(tty, state, NULL);
> spin_lock_irq(&uport->lock);
> - ops->set_mctrl(uport, uport->mctrl);
> + if (!(uport->rs485.flags & SER_RS485_ENABLED))
> + ops->set_mctrl(uport, uport->mctrl);
> + else
> + uart_rs485_config(uport);
> ops->start_tx(uport);
> spin_unlock_irq(&uport->lock);
> tty_port_set_initialized(port, 1);
> @@ -2558,10 +2562,10 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *uport)
> */
> spin_lock_irqsave(&port->lock, flags);
> port->mctrl &= TIOCM_DTR;
> - if (port->rs485.flags & SER_RS485_ENABLED &&
> - !(port->rs485.flags & SER_RS485_RTS_AFTER_SEND))
> - port->mctrl |= TIOCM_RTS;
> - port->ops->set_mctrl(port, port->mctrl);
> + if (!(port->rs485.flags & SER_RS485_ENABLED))
> + port->ops->set_mctrl(port, port->mctrl);
> + else
> + uart_rs485_config(port);
> spin_unlock_irqrestore(&port->lock, flags);
>
> /*
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 02a4299..0354369 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -933,5 +933,4 @@ static inline int uart_handle_break(struct uart_port *port)
> !((cflag) & CLOCAL))
>
> int uart_get_rs485_mode(struct uart_port *port);
> -int uart_rs485_config(struct uart_port *port);
> #endif /* LINUX_SERIAL_CORE_H */
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] serial: Deassert RS485 Transmit Enable on probe in driver-specific way
[not found] <b17842d9792762948f75d74205596d36bdd27f22.1663310999.git.lukas@wunner.de>
2022-09-16 11:22 ` [PATCH] serial: Deassert RS485 Transmit Enable on probe in driver-specific way Matthias Schiffer
@ 2022-09-16 12:27 ` Ilpo Järvinen
2022-09-17 14:25 ` Lukas Wunner
1 sibling, 1 reply; 3+ messages in thread
From: Ilpo Järvinen @ 2022-09-16 12:27 UTC (permalink / raw)
To: Lukas Wunner
Cc: Greg Kroah-Hartman, Matthias Schiffer, Roosen Henri,
linux-serial, Jiri Slaby, Lino Sanfilippo, David Laight,
Maarten Brock, Jan Kiszka, Su Bao Cheng, baocheng_su, Chao Zeng,
Peter Hung, Joachim Eastwood, Daniel Golle, Codrin Ciubotariu,
Sherry Sun, Serge Semin, Ricardo Ribalda, Dario Binacchi,
Bich Hemon, Marek Vasut
On Fri, 16 Sep 2022, Lukas Wunner wrote:
> When a UART port is newly registered, uart_configure_port() seeks to
> deassert RS485 Transmit Enable by setting the RTS bit in port->mctrl.
> However a number of UART drivers interpret a set RTS bit as *assertion*
> instead of deassertion: Affected drivers include those using
> serial8250_em485_config() (except 8250_bcm2835aux.c) and some using
> mctrl_gpio (e.g. imx.c).
>
> Since the interpretation of the RTS bit is driver-specific, it is not
> suitable as a means to centrally deassert Transmit Enable in the serial
> core. Instead, the serial core must call on drivers to deassert it in
> their driver-specific way. One way to achieve that is to call
> ->rs485_config(). It implicitly deasserts Transmit Enable.
>
> So amend uart_configure_port() and uart_resume_port() to invoke
> uart_rs485_config(). That allows removing calls to uart_rs485_config()
> from drivers' ->probe() hooks and declaring the function static.
>
> Skip any invocation of ->set_mctrl() if RS485 is enabled. RS485 has no
> hardware flow control, so the modem control lines are irrelevant and
> need not be touched. When leaving RS485 mode, reset the modem control
> lines to the state stored in port->mctrl. That way, UARTs which are
> muxed between RS485 and RS232 transceivers drive the lines correctly
> when switched to RS232. (serial8250_do_startup() historically raises
> the OUT1 modem signal because otherwise interrupts are not signaled on
> ancient PC UARTs, but I believe that no longer applies to modern,
> RS485-capable UARTs and is thus safe to be skipped.)
>
> imx.c modifies port->mctrl whenever Transmit Enable is asserted and
> deasserted. Stop it from doing that so port->mctrl reflects the RS232
> line state.
>
> 8250_omap.c deasserts Transmit Enable on ->runtime_resume() by calling
> ->set_mctrl(). Because that is now a no-op in RS485 mode, amend the
> function to call serial8250_em485_stop_tx().
>
> fsl_lpuart.c retrieves and applies the RS485 device tree properties
> after registering the UART port. Because applying now happens on
> registration in uart_configure_port(), move retrieval of the properties
> ahead of uart_add_one_port().
>
> Fixes: d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart open")
> Reported-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> Link: https://lore.kernel.org/all/20220329085050.311408-1-matthias.schiffer@ew.tq-group.com/
> Reported-by: Roosen Henri <Henri.Roosen@ginzinger.com>
> Link: https://lore.kernel.org/all/8f538a8903795f22f9acc94a9a31b03c9c4ccacb.camel@ginzinger.com/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org # v4.14+
> ---
> Based on v6.0-rc3 + this dependency:
> https://lore.kernel.org/linux-serial/72fb646c1b0b11c989850c55f52f9ff343d1b2fa.1662884345.git.lukas@wunner.de/
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 907c5ff..a6f03b1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -600,7 +600,7 @@ void serial8250_rpm_put(struct uart_8250_port *p)
> static int serial8250_em485_init(struct uart_8250_port *p)
> {
> if (p->em485)
> - return 0;
> + goto deassert_rts;
>
> p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_ATOMIC);
> if (!p->em485)
> @@ -616,6 +616,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
> p->em485->active_timer = NULL;
> p->em485->tx_stopped = true;
>
> +deassert_rts:
> p->rs485_stop_tx(p);
if (p->em485->tx_stopped)
p->rs485_stop_tx(p);
?
Because if p->em485->tx_stopped is false and serial8250_em485_init() is
called again (the comment above it says it is safe to do so), it would
stop tx at wrong point.
--
i.
^ permalink raw reply [flat|nested] 3+ messages in thread