Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b2f29129f966685105e09781620b85c8f4f1a88e.camel@ew.tq-group.com>
Subject: Re: [PATCH] serial: Revert RS485 polarity change on UART open
From:   Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To:     Lukas Wunner <lukas@wunner.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ilpo =?ISO-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Lino Sanfilippo <LinoSanfilippo@gmx.de>
Date:   Tue, 29 Mar 2022 12:39:02 +0200
In-Reply-To: <20220329100328.GA2090@wunner.de>
References: <20220329085050.311408-1-matthias.schiffer@ew.tq-group.com>
         <20220329100328.GA2090@wunner.de>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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