While the change of the RS485 polarity in
commit d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart open")
might have made sense based on the original intention of the
rs485-rts-active-low flag (*), this is not how it is implemented in
various drivers:
At least the 8250 and the i.MX UART drivers interpret rs485-rts-active-low
the other way around. For these drivers, said change broke the initial
state of the RTS pin after opening the UART, making it impossible to
receive data until after the first send.
I considered fixing up the drivers to match the polarity used for the
initial state, however doing so would break all existing Device Trees
that configure RS485 for one of these drivers. Reverting the change
makes RS485 work correctly again on these devices.
[(*) My understanding of the mentioned commit's description is that
rs485-rts-active-low should have referred to the electical signal level
of the RTS pin, rather than the logical RTS state as understood by the
UART controller.]
Fixes: d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart open")
Fixes: 2dd8a74fddd2 ("serial: core: Initialize rs485 RTS polarity already on probe")
Signed-off-by: Matthias Schiffer <[email protected]>
---
Skimming through a few other drivers with RS485 support, many other
implementation are in agreement with the 8250 and i.MX drivers. This seems
to be the case for the OMAP and the STM32 drivers. The PL011 driver on the
other hand seems to disagree, so it will need to be fixed if the intention
is to make all drivers consistent (preferable by someone who is familiar
with that hardware and can test the change).
It is unfortunate that different drivers disagree here, as any fix to
make things more consistent will break some users. One way to avoid that
would be to deprecate the rs485-rts-active-low flag altogether and replace
it with something new that is implemented consistently...
I can also propose a clarification for the DT binding documentation, if
this revert is how we want to proceed with the issue.
drivers/tty/serial/serial_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6a8963caf954..c1c8bd712a59 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2390,7 +2390,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
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->rs485.flags & SER_RS485_RTS_ON_SEND))
port->mctrl |= TIOCM_RTS;
port->ops->set_mctrl(port, port->mctrl);
spin_unlock_irqrestore(&port->lock, flags);
--
2.25.1
[cc += Ilpo, Lino]
On Tue, Mar 29, 2022 at 10:50:50AM +0200, Matthias Schiffer wrote:
> While the change of the RS485 polarity in
> commit d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart open")
> might have made sense based on the original intention of the
> rs485-rts-active-low flag (*), this is not how it is implemented in
> various drivers:
[...]
> [(*) My understanding of the mentioned commit's description is that
> rs485-rts-active-low should have referred to the electical signal level
> of the RTS pin, rather than the logical RTS state as understood by the
> UART controller.]
Since RTS is often just a GPIO on a pin controller that's configured
to function as RTS, my expectation would be that the same rules apply
to RTS polarity as those that apply to *any* GPIO.
According to Documentation/devicetree/bindings/gpio/gpio.txt:
"A gpio-specifier should contain a flag indicating the GPIO polarity; active-
high or active-low. If it does, the following best practices should be
followed:
The gpio-specifier's polarity flag should represent the physical level at the
^^^^^^^^^^^^^^
GPIO controller that achieves (or represents, for inputs) a logically asserted
value at the device."
> At least the 8250 and the i.MX UART drivers interpret rs485-rts-active-low
Which 8250 driver are you referring to specifically? When developing
d3b3404df318, I tested with 8250_bcm2835aux.c and amba-pl011.c. Both
worked exactly the way they should.
If imx.c and others have historically interpreted rs485-rts-active-low
to mean that the physical level is "high" when active, then we could just
amend imx_uart_probe() such that after calling uart_get_rs485_mode(),
the SER_RS485_RTS_ON_SEND and SER_RS485_RTS_AFTER_SEND bits are
flipped. Would that work for you?
I'll go through the drivers to check which ones are affected. I'm sorry
that you're seeing breakage, it's surprising to me that these different
interpretations of rs485-rts-active-low exist.
Thanks,
Lukas
On Tue, 2022-03-29 at 12:03 +0200, Lukas Wunner wrote:
> [cc += Ilpo, Lino]
>
> On Tue, Mar 29, 2022 at 10:50:50AM +0200, Matthias Schiffer wrote:
> > While the change of the RS485 polarity in
> > commit d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart
> > open")
> > might have made sense based on the original intention of the
> > rs485-rts-active-low flag (*), this is not how it is implemented in
> > various drivers:
> [...]
> > [(*) My understanding of the mentioned commit's description is that
> > rs485-rts-active-low should have referred to the electical signal
> > level
> > of the RTS pin, rather than the logical RTS state as understood by
> > the
> > UART controller.]
Hi Lukas,
>
> Since RTS is often just a GPIO on a pin controller that's configured
> to function as RTS, my expectation would be that the same rules apply
> to RTS polarity as those that apply to *any* GPIO.
>
> According to Documentation/devicetree/bindings/gpio/gpio.txt:
>
> "A gpio-specifier should contain a flag indicating the GPIO polarity;
> active-
> high or active-low. If it does, the following best practices should
> be
> followed:
> The gpio-specifier's polarity flag should represent the physical
> level at the
> ^^^^^^^^^^^^
> ^^
> GPIO controller that achieves (or represents, for inputs) a
> logically asserted
> value at the device."
Yes, that would make sense to me as well, but as described, this is not
how the majority of drivers that I looked at works at the moment :(
I'm not particularly attached to any of the interpretations, but it
would be great to have some consistency here. And if the majority of
drivers does it the "wrong" way, maybe we should accept that to keep
the breakage as small as possible?
>
>
> > At least the 8250 and the i.MX UART drivers interpret rs485-rts-
> > active-low
>
> Which 8250 driver are you referring to specifically? When developing
> d3b3404df318, I tested with 8250_bcm2835aux.c and amba-pl011.c. Both
> worked exactly the way they should.
I tested with 8250_omap.c, which does not implement the RS485 handling
itself, but refers to the generic code in 8250_port.c. In fact, my
first attempt to get the RS485 to work on my device was the following
(which matches the originally intended interpretation of the polarity
flag, but breaks existing users):
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1460,9 +1460,9 @@ void serial8250_em485_stop_tx(struct
uart_8250_port *p)
unsigned char mcr = serial8250_in_MCR(p);
if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
- mcr |= UART_MCR_RTS;
- else
mcr &= ~UART_MCR_RTS;
+ else
+ mcr |= UART_MCR_RTS;
serial8250_out_MCR(p, mcr);
/*
@@ -1611,9 +1611,9 @@ void serial8250_em485_start_tx(struct
uart_8250_port *up)
serial8250_stop_rx(&up->port);
if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
- mcr |= UART_MCR_RTS;
- else
mcr &= ~UART_MCR_RTS;
+ else
+ mcr |= UART_MCR_RTS;
serial8250_out_MCR(up, mcr);
}
>
> If imx.c and others have historically interpreted rs485-rts-active-
> low
> to mean that the physical level is "high" when active, then we could
> just
> amend imx_uart_probe() such that after calling uart_get_rs485_mode(),
> the SER_RS485_RTS_ON_SEND and SER_RS485_RTS_AFTER_SEND bits are
> flipped. Would that work for you?
>
I guess that would work. The fact that even the different
variants of the 8250 are implemented inconsistently makes this
especially ugly... It certainly puts a damper on the efforts to make
the handling of RS485 in serial drivers more generic.
> I'll go through the drivers to check which ones are affected. I'm
> sorry
> that you're seeing breakage, it's surprising to me that these
> different
> interpretations of rs485-rts-active-low exist.
>
> Thanks,
>
> Lukas
Regards,
Matthias
On Tue, 2022-03-29 at 12:39 +0200, Matthias Schiffer wrote:
> On Tue, 2022-03-29 at 12:03 +0200, Lukas Wunner wrote:
> > [cc += Ilpo, Lino]
> >
> > On Tue, Mar 29, 2022 at 10:50:50AM +0200, Matthias Schiffer wrote:
> > > While the change of the RS485 polarity in
> > > commit d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart
> > > open")
> > > might have made sense based on the original intention of the
> > > rs485-rts-active-low flag (*), this is not how it is implemented in
> > > various drivers:
> > [...]
> > > [(*) My understanding of the mentioned commit's description is that
> > > rs485-rts-active-low should have referred to the electical signal
> > > level
> > > of the RTS pin, rather than the logical RTS state as understood by
> > > the
> > > UART controller.]
>
> Hi Lukas,
>
> > Since RTS is often just a GPIO on a pin controller that's configured
> > to function as RTS, my expectation would be that the same rules apply
> > to RTS polarity as those that apply to *any* GPIO.
> >
> > According to Documentation/devicetree/bindings/gpio/gpio.txt:
> >
> > "A gpio-specifier should contain a flag indicating the GPIO polarity;
> > active-
> > high or active-low. If it does, the following best practices should
> > be
> > followed:
> > The gpio-specifier's polarity flag should represent the physical
> > level at the
> > ^^^^^^^^^^^^
> > ^^
> > GPIO controller that achieves (or represents, for inputs) a
> > logically asserted
> > value at the device."
>
> Yes, that would make sense to me as well, but as described, this is not
> how the majority of drivers that I looked at works at the moment :(
>
> I'm not particularly attached to any of the interpretations, but it
> would be great to have some consistency here. And if the majority of
> drivers does it the "wrong" way, maybe we should accept that to keep
> the breakage as small as possible?
>
> >
> > > At least the 8250 and the i.MX UART drivers interpret rs485-rts-
> > > active-low
> >
> > Which 8250 driver are you referring to specifically? When developing
> > d3b3404df318, I tested with 8250_bcm2835aux.c and amba-pl011.c. Both
> > worked exactly the way they should.
>
> I tested with 8250_omap.c, which does not implement the RS485 handling
> itself, but refers to the generic code in 8250_port.c. In fact, my
> first attempt to get the RS485 to work on my device was the following
> (which matches the originally intended interpretation of the polarity
> flag, but breaks existing users):
>
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1460,9 +1460,9 @@ void serial8250_em485_stop_tx(struct
> uart_8250_port *p)
> unsigned char mcr = serial8250_in_MCR(p);
>
> if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
> - mcr |= UART_MCR_RTS;
> - else
> mcr &= ~UART_MCR_RTS;
> + else
> + mcr |= UART_MCR_RTS;
> serial8250_out_MCR(p, mcr);
>
> /*
> @@ -1611,9 +1611,9 @@ void serial8250_em485_start_tx(struct
> uart_8250_port *up)
> serial8250_stop_rx(&up->port);
>
> if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
> - mcr |= UART_MCR_RTS;
> - else
> mcr &= ~UART_MCR_RTS;
> + else
> + mcr |= UART_MCR_RTS;
> serial8250_out_MCR(up, mcr);
> }
>
> > If imx.c and others have historically interpreted rs485-rts-active-
> > low
> > to mean that the physical level is "high" when active, then we could
> > just
> > amend imx_uart_probe() such that after calling uart_get_rs485_mode(),
> > the SER_RS485_RTS_ON_SEND and SER_RS485_RTS_AFTER_SEND bits are
> > flipped. Would that work for you?
> >
>
> I guess that would work. The fact that even the different
> variants of the 8250 are implemented inconsistently makes this
> especially ugly... It certainly puts a damper on the efforts to make
> the handling of RS485 in serial drivers more generic.
>
>
> > I'll go through the drivers to check which ones are affected. I'm
> > sorry
> > that you're seeing breakage, it's surprising to me that these
> > different
> > interpretations of rs485-rts-active-low exist.
> > Thanks,
> >
> > Lukas
>
Hi Lukas,
do you know if there has been any progress on this issue? I see that
there has been quite a bit of activity in the RS485 code in linux-next,
but I didn't have time to check if that has any effect on the polarity
issue so far.
Thanks,
Matthias
On Mon, Jun 27, 2022 at 10:40:36AM +0200, Matthias Schiffer wrote:
> On Tue, 2022-03-29 at 12:39 +0200, Matthias Schiffer wrote:
> > On Tue, 2022-03-29 at 12:03 +0200, Lukas Wunner wrote:
> > > On Tue, Mar 29, 2022 at 10:50:50AM +0200, Matthias Schiffer wrote:
> > > > While the change of the RS485 polarity in
> > > > commit d3b3404df318 ("serial: Fix incorrect rs485 polarity on uart
> > > > open")
> > > > might have made sense based on the original intention of the
> > > > rs485-rts-active-low flag (*), this is not how it is implemented in
> > > > various drivers:
> > > [...]
> > > > [(*) My understanding of the mentioned commit's description is that
> > > > rs485-rts-active-low should have referred to the electical signal
> > > > level
> > > > of the RTS pin, rather than the logical RTS state as understood by
> > > > the
> > > > UART controller.]
>
> do you know if there has been any progress on this issue? I see that
> there has been quite a bit of activity in the RS485 code in linux-next,
> but I didn't have time to check if that has any effect on the polarity
> issue so far.
No, the issue has not been resolved yet but at least I have inspected
all drivers now and compiled a list of their behavior:
8250_bcm2835aux: identity (RTS in software)
8250_dwlib: identity (RTS in hardware, DE active high after POR)
inverse (RTS in software em485)
8250_exar: identity (RTS in hardware)
8250_fintek: inverse (RTS in hardware)
8250_lpc18xx: identity (RTS in hardware)
8250_of: inverse (RTS in software em485)
[ns8250, ns16550, lpc3220, da830, wpcm450]
8250_omap: inverse (RTS in software em485)
8250_pci: identity (RTS in hardware)
amba-pl011: identity (RTS in software)
ar933x_uart: identity (RTS as gpio)
atmel_serial: identity (RTS in hardware, only high supported)
fsl_lpuart: inverse (RTS in hardware)
imx: identity (RTS as gpio or CTS in software)
max310x: identity (RTS in hardware)
mcf: identity (RTS in hardware)
omap-serial: identity (RTS as gpio)
sc16is7xx: inverse (RTS in hardware)
stm32-usart: identity (RTS in hardware)
identity (RTS as gpio)
Where "identity" means that SER_RS485_RTS_ON_SEND results in "high"
voltage level and "inverse" means it results in "low" voltage level.
Unless I've made any mistakes here, it looks like the majority of
drivers use "identity". The only ones using inverse polarity are
8250-compatible UARTs using em485 software emulation (with the
exception of bcm2835aux), plus three UART drivers which use
hardware-driven RTS.
When you reported the issue, you claimed that i.MX, OMAP and STM32
drivers use inverse polarity. I was only able to confirm that for
OMAP (if 8250_omap.c is used, not the older omap-serial.c).
When you refer to i.MX, I suppose you're referring to fsl_lpuart.c
(which uses hardware-controlled RTS assertion on newer i.MX SoCs),
not imx.c, right? The latter offers two options to drive an RS-485
transceiver, either through CTS or through an mctrl_gpio.
imx_uart_rts_active() clears the CTS bit in the UCR2 register.
According to page 4679 of iMX53RM.pdf, that means "high" voltage level,
hence "identity". And if an mctrl_gpio is used, mctrl_gpio_set()
sets the GPIO to "active", which likewise means "high" voltage level
unless you've specified the GPIO as active-low in the devicetree:
https://www.nxp.com/docs/en/reference-manual/iMX53RM.pdf
As for stm32-usart.c, the driver offers either hardware-controlled
RTS assertion or mctrl_gpio. The hardware-controlled variant clears
the DEP bit in the USART_CR3 register if SER_RS485_RTS_ON_SEND is set.
According to page 1392 of the STM32 documentation this means that
"DE signal is active high", so it seems to me that this driver uses
"identity and not "inverse" as claimed:
https://www.st.com/resource/en/reference_manual/rm0351-stm32l47xxx-stm32l48xxx-stm32l49xxx-and-stm32l4axxx-advanced-armbased-32bit-mcus-stmicroelectronics.pdf
Anyway, I'll try to come up with a fix for the affected em485 drivers
by next week. The "inverse" drivers with hardware-controlled RTS
should not be affected by d3b3404df318 (unless I'm missing something).
Thanks!
Lukas