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: <383f5d66d6828a5f09f5705f6ff98e3727ecfdf2.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:   Mon, 27 Jun 2022 10:40:36 +0200
In-Reply-To: <b2f29129f966685105e09781620b85c8f4f1a88e.camel@ew.tq-group.com>
References: <20220329085050.311408-1-matthias.schiffer@ew.tq-group.com>
         <20220329100328.GA2090@wunner.de>
         <b2f29129f966685105e09781620b85c8f4f1a88e.camel@ew.tq-group.com>
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: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