Re: [PATCH 1/7] serial: 8250_dwlib: RS485 HW half duplex support

From: Lukas Wunner <lukas@wunner.de>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: linux-serial <linux-serial@vger.kernel.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Johan Hovold <johan@kernel.org>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Raymond Tan <raymond.tan@intel.com>,
	Heiko Stuebner <heiko@sntech.de>, Rob Herring <robh@kernel.org>
Subject: Re: [PATCH 1/7] serial: 8250_dwlib: RS485 HW half duplex support
Date: Wed, 9 Mar 2022 13:59:08 +0100	[thread overview]
Message-ID: <20220309125908.GA9283@wunner.de> (raw)
In-Reply-To: <9c2d96c0-44cf-c84c-5ff7-34c74716a23b@linux.intel.com>

On Wed, Mar 09, 2022 at 02:19:39PM +0200, Ilpo Järvinen wrote:
> Now that it has been pretty much established that anything called "rts" 
> should be applied to DE as well, I took another look on implementing these 
> delays.
> 
> It turns out to be impractical to do/ineffective because "serial clock 
> periods" are used as the unit by the HW ("serial clock periods" is not as 
> clearly defined by the datasheet as I'd like but it is most likely based 
> on the high-rated uartclk cycles). With the uartclk I've on test HW, the 
> combined delay with max turnaround time and DE assert/de-assert timings 
> cannot do even the smallest possible non-zero value (1 msec). That's 
> because the TAT and DET registers allow only 16-bit and 8-bit values for 
> delays.

A mistake was made when RS-485 support was introduced in the kernel
more than 10 years ago with commits c26c56c0 and 93f3350c:

The delays were defined in msec, but if you look at datasheets of
RS-485 transceivers, they only need a delay in the nanosecond or
single-digit usec range.

Here's a collection of delays I compiled two years ago:

                                 DrvEnable-to-Output    DriverPropagation
          MAX13450E/MAX13451E    5200 ns                 800 ns
          MAXM22510              2540 ns                1040 ns
          MAXM22511                80 ns                  65 ns
          SN65HVD72              9000 ns                1000 ns
          SN65HVD75              7000 ns                  17 ns
          SN65HVD78              8000 ns                  15 ns
          SN65HVD485E            2600 ns                  30 ns
          ADM1486                  15 ns                  17 ns
          ADM3485/ADM3490/ADM3491 900 ns                  35 ns
          ADM3483/ADM3488        1300 ns                1500 ns
          XR33193                2000 ns                1300 ns

Because these delays are so short, it is usually sufficient to set
them to zero in struct serial_rs485.

I've begun a commit to change the delays to nsec, it's still a WIP:

https://github.com/l1k/linux/commit/2c08878b63d6

This is a little tricky because the delays are user-space ABI,
so great care is needed to avoid breakage.  Also, every rs485
driver with support for delays needs to be touched.  Some
UARTs have fixed delays which depend on different clocks,
other UARTs support configurable delays.  Another complication
is that delay calculations easily overflow with nsec because
the numbers become quite large.

A positive side effect of changing the delays to nsec is that
the horrible hrtimer kludge for rs485 software emulation in the
8250_port.c can be eliminated.  Also, all the illegal mdelay()s
in spinlocked context (e.g. serial console output) are replaced
by much more reasonable ndelay()s.

Eliminating the hrtimer kludge in 8250_port.c might also make these
runtime PM patches by Andy simpler:

https://lore.kernel.org/linux-serial/20211115084203.56478-8-tony@atomide.com/

My suggestion would be to set the delays to zero for now in 8250_dw.c
and implement proper delay handling after I've finished the conversion
to nsec.

Thanks,

Lukas