All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] serial: Revert RS485 polarity change on UART open
@ 2022-03-29  8:50 Matthias Schiffer
  2022-03-29 10:03 ` Lukas Wunner
  0 siblings, 1 reply; 10+ messages in thread
From: Matthias Schiffer @ 2022-03-29  8:50 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby, Lukas Wunner
  Cc: Russell King, linux-serial, linux-kernel, Matthias Schiffer

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 <matthias.schiffer@ew.tq-group.com>
---

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-03-29  8:50 [PATCH] serial: Revert RS485 polarity change on UART open Matthias Schiffer
@ 2022-03-29 10:03 ` Lukas Wunner
  2022-03-29 10:39   ` Matthias Schiffer
  0 siblings, 1 reply; 10+ messages in thread
From: Lukas Wunner @ 2022-03-29 10:03 UTC (permalink / raw)
  To: Matthias Schiffer
  Cc: Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo

[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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-03-29 10:03 ` Lukas Wunner
@ 2022-03-29 10:39   ` Matthias Schiffer
  2022-03-29 12:55     ` David Laight
  2022-06-27  8:40     ` Matthias Schiffer
  0 siblings, 2 replies; 10+ messages in thread
From: Matthias Schiffer @ 2022-03-29 10:39 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo

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




^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-03-29 10:39   ` Matthias Schiffer
@ 2022-03-29 12:55     ` David Laight
  2022-03-29 13:02       ` Matthias Schiffer
  2022-06-27  8:40     ` Matthias Schiffer
  1 sibling, 1 reply; 10+ messages in thread
From: David Laight @ 2022-03-29 12:55 UTC (permalink / raw)
  To: 'Matthias Schiffer', Lukas Wunner
  Cc: Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo

From: Matthias Schiffer
> Sent: 29 March 2022 11:39
...
> 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.

One thing to remember is that RS232 (IIRC really V.38) line driver
chips are typically inverting.

So the modem signals on a TTL level output will have the
opposite polarity to that required on the actual connector.

Normally a UART will have an 'active high' register bit for
a modem signal that drives and 'active low' pin so you get
the correct polarity with an inverting line driver.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-03-29 12:55     ` David Laight
@ 2022-03-29 13:02       ` Matthias Schiffer
  2022-03-29 13:19         ` David Laight
  0 siblings, 1 reply; 10+ messages in thread
From: Matthias Schiffer @ 2022-03-29 13:02 UTC (permalink / raw)
  To: David Laight
  Cc: Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo, Lukas Wunner

On Tue, 2022-03-29 at 12:55 +0000, David Laight wrote:
> From: Matthias Schiffer
> > Sent: 29 March 2022 11:39
> ...
> > 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.
> 
> One thing to remember is that RS232 (IIRC really V.38) line driver
> chips are typically inverting.
> 
> So the modem signals on a TTL level output will have the
> opposite polarity to that required on the actual connector.
> 
> Normally a UART will have an 'active high' register bit for
> a modem signal that drives and 'active low' pin so you get
> the correct polarity with an inverting line driver.
> 
> 	David
> 

Indeed. As far as I can tell, this property of UARTs is what got us
into this mess: Some people interpreted SER_RS485_RTS_ON_SEND as "set
the RTS flag in the MCR register on send", while other thought it
should mean "set the RTS pin to high on send", leading to opposite
behaviours in different UART drivers (and even different UART variants
in the same driver, in the case of the 8250 family).

Regards,
Matthias


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-03-29 13:02       ` Matthias Schiffer
@ 2022-03-29 13:19         ` David Laight
  2022-03-29 13:36           ` (EXT) " Matthias Schiffer
  2022-05-11 10:22           ` m.brock
  0 siblings, 2 replies; 10+ messages in thread
From: David Laight @ 2022-03-29 13:19 UTC (permalink / raw)
  To: 'Matthias Schiffer'
  Cc: Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo, Lukas Wunner

From: Matthias Schiffer
> Sent: 29 March 2022 14:03
> 
> On Tue, 2022-03-29 at 12:55 +0000, David Laight wrote:
> > From: Matthias Schiffer
> > > Sent: 29 March 2022 11:39
> > ...
> > > 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.
> >
> > One thing to remember is that RS232 (IIRC really V.38) line driver
> > chips are typically inverting.
> >
> > So the modem signals on a TTL level output will have the
> > opposite polarity to that required on the actual connector.
> >
> > Normally a UART will have an 'active high' register bit for
> > a modem signal that drives and 'active low' pin so you get
> > the correct polarity with an inverting line driver.
> >
> > 	David
> >
> 
> Indeed. As far as I can tell, this property of UARTs is what got us
> into this mess: Some people interpreted SER_RS485_RTS_ON_SEND as "set
> the RTS flag in the MCR register on send", while other thought it
> should mean "set the RTS pin to high on send", leading to opposite
> behaviours in different UART drivers (and even different UART variants
> in the same driver, in the case of the 8250 family).

Hmmm... A complete mess.
The 'RTS pin' that needs to go high is the one on the (typically) 'D'
connector after the inverting line driver.
Not the pin on the uart package.
I'd expect TTL level serial interfaces to require active low
modem signals.

If RS485 is trying to do half duplex using RTS (request to send)
and CTS (clear to send) you've typically got bigger problems
than asserting RTS before a transmit.
The real problem is removing RTS once the last transmit data bit
(the stop bit) has left the UART pin.
I've used local loopback (tx to rx) to detect that in the past.

Of course, if it is just doing flow control that should use RFS
(ready for sending) to indicate space in the receive fifo but
using the RTS pin instead that is a different matter.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: (EXT) RE: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-03-29 13:19         ` David Laight
@ 2022-03-29 13:36           ` Matthias Schiffer
  2022-05-11 10:22           ` m.brock
  1 sibling, 0 replies; 10+ messages in thread
From: Matthias Schiffer @ 2022-03-29 13:36 UTC (permalink / raw)
  To: David Laight
  Cc: Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo, Lukas Wunner

On Tue, 2022-03-29 at 13:19 +0000, David Laight wrote:
> From: Matthias Schiffer
> > Sent: 29 March 2022 14:03
> > 
> > On Tue, 2022-03-29 at 12:55 +0000, David Laight wrote:
> > > From: Matthias Schiffer
> > > > Sent: 29 March 2022 11:39
> > > ...
> > > > 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.
> > > 
> > > One thing to remember is that RS232 (IIRC really V.38) line
> > > driver
> > > chips are typically inverting.
> > > 
> > > So the modem signals on a TTL level output will have the
> > > opposite polarity to that required on the actual connector.
> > > 
> > > Normally a UART will have an 'active high' register bit for
> > > a modem signal that drives and 'active low' pin so you get
> > > the correct polarity with an inverting line driver.
> > > 
> > > 	David
> > > 
> > 
> > Indeed. As far as I can tell, this property of UARTs is what got us
> > into this mess: Some people interpreted SER_RS485_RTS_ON_SEND as
> > "set
> > the RTS flag in the MCR register on send", while other thought it
> > should mean "set the RTS pin to high on send", leading to opposite
> > behaviours in different UART drivers (and even different UART
> > variants
> > in the same driver, in the case of the 8250 family).
> 
> Hmmm... A complete mess.
> The 'RTS pin' that needs to go high is the one on the (typically) 'D'
> connector after the inverting line driver.
> Not the pin on the uart package.
> I'd expect TTL level serial interfaces to require active low
> modem signals.
> 
> If RS485 is trying to do half duplex using RTS (request to send)
> and CTS (clear to send) you've typically got bigger problems
> than asserting RTS before a transmit.
> The real problem is removing RTS once the last transmit data bit
> (the stop bit) has left the UART pin.
> I've used local loopback (tx to rx) to detect that in the past.
> 
> Of course, if it is just doing flow control that should use RFS
> (ready for sending) to indicate space in the receive fifo but
> using the RTS pin instead that is a different matter.
> 
> 	David
> 


I'm aware of the difficulties of deasserting RTS after the transmission
is complete, but that's completely orthogonal to the issue I'm trying
to solve right now :)

Regards,
Matthias


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-03-29 13:19         ` David Laight
  2022-03-29 13:36           ` (EXT) " Matthias Schiffer
@ 2022-05-11 10:22           ` m.brock
  1 sibling, 0 replies; 10+ messages in thread
From: m.brock @ 2022-05-11 10:22 UTC (permalink / raw)
  To: David Laight
  Cc: 'Matthias Schiffer',
	Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo, Lukas Wunner

David Laight wrote on 2022-03-29 15:19:
> From: Matthias Schiffer
>> Sent: 29 March 2022 14:03
>> 
>> On Tue, 2022-03-29 at 12:55 +0000, David Laight wrote:
>> > From: Matthias Schiffer
>> > > Sent: 29 March 2022 11:39
>> > ...
>> > > 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.
>> >
>> > One thing to remember is that RS232 (IIRC really V.38) line driver
>> > chips are typically inverting.
>> >
>> > So the modem signals on a TTL level output will have the
>> > opposite polarity to that required on the actual connector.
>> >
>> > Normally a UART will have an 'active high' register bit for
>> > a modem signal that drives and 'active low' pin so you get
>> > the correct polarity with an inverting line driver.
>> >
>> > 	David
>> >
>> 
>> Indeed. As far as I can tell, this property of UARTs is what got us
>> into this mess: Some people interpreted SER_RS485_RTS_ON_SEND as "set
>> the RTS flag in the MCR register on send", while other thought it
>> should mean "set the RTS pin to high on send", leading to opposite
>> behaviours in different UART drivers (and even different UART variants
>> in the same driver, in the case of the 8250 family).
> 
> Hmmm... A complete mess.
> The 'RTS pin' that needs to go high is the one on the (typically) 'D'
> connector after the inverting line driver.
> Not the pin on the uart package.
> I'd expect TTL level serial interfaces to require active low
> modem signals.

A typical 'D' connector for RS485? I've seen all sorts of connectors
and I can't say one of them is typical. You're assuming there are
RS232 level shifters in play, but usually there aren't and the TTL
UART signals are directly connected to an RS485 transceiver. For
RS485 there is no outside RTS pin, so you will have to talk about
the UART TTL pin.

https://www.kernel.org/doc/Documentation/serial/serial-rs485.txt
is unfortunately not entirely clear:
	/* Set logical level for RTS pin equal to 1 when sending: */
	rs485conf.flags |= SER_RS485_RTS_ON_SEND;

Most (all?) UART chips have an RTSn, RTS# or /RTS pin which means RTS
inverted. So the "RTS pin" can't be set to logic level 1, but RTS can.

IMHO it would be best to let SER_RS485_RTS_ON_SEND mean that RTS is
asserted during sending. That means that RTSn should be 0V.

When the UART chip is connected directly to an RS485 driver with RTSn
connected to DE, you need SER_RS485_RTS_AFTER_SEND so RTS=0 and
RTSn=DE=1 during sending.

When a UART is not in RS485 mode and unopened it usually has RTS=0,
RTSn=1 which would make any directly connected RS485 driver claim
the bus. This is probably undesired behaviour and thus asks for an
extra inverter between RTSn and DE. With this added inverter one
would need SER_RS485_RTS_ON_SEND.

https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/serial/rs485.yaml
isn't much better either:
  rs485-rts-active-low:
     description: drive RTS low when sending (default is high).

Was this meant as "drive the RTSn pin low when sending"?

Finally, can these two methods be synchronized in their wording?
E.g. can we just drop "rs485-rts-active-low" and replace it with
"rs485-rts-on-send" and "rs485-rts-after-send"?

Maarten


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-03-29 10:39   ` Matthias Schiffer
  2022-03-29 12:55     ` David Laight
@ 2022-06-27  8:40     ` Matthias Schiffer
  2022-06-29 17:01       ` Lukas Wunner
  1 sibling, 1 reply; 10+ messages in thread
From: Matthias Schiffer @ 2022-06-27  8:40 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] serial: Revert RS485 polarity change on UART open
  2022-06-27  8:40     ` Matthias Schiffer
@ 2022-06-29 17:01       ` Lukas Wunner
  0 siblings, 0 replies; 10+ messages in thread
From: Lukas Wunner @ 2022-06-29 17:01 UTC (permalink / raw)
  To: Matthias Schiffer
  Cc: Greg Kroah-Hartman, Jiri Slaby, Russell King, linux-serial,
	linux-kernel, Ilpo Järvinen, Lino Sanfilippo, m.brock,
	David Laight

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-06-29 17:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-29  8:50 [PATCH] serial: Revert RS485 polarity change on UART open Matthias Schiffer
2022-03-29 10:03 ` Lukas Wunner
2022-03-29 10:39   ` Matthias Schiffer
2022-03-29 12:55     ` David Laight
2022-03-29 13:02       ` Matthias Schiffer
2022-03-29 13:19         ` David Laight
2022-03-29 13:36           ` (EXT) " Matthias Schiffer
2022-05-11 10:22           ` m.brock
2022-06-27  8:40     ` Matthias Schiffer
2022-06-29 17:01       ` Lukas Wunner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.