Re: [PATCH v3 00/12] Add RS485 support to DW UART

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3972 bytes --]

On Thu, 21 Apr 2022, Vicente Bergas wrote:

> i have tested your v3 patch on v3 hardware, that is, using the
> emulated em485 because of lack of HW support. It is not working
> due to three issues.

Thanks for testing!

> 1.- rs485_stop_tx is never called because there are no interrupts.
> I worked around this by disabling DMA:
> 
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -577,3 +577,3 @@ static int dw8250_probe(struct platform_device *pdev)
>  		data->data.dma.rxconf.src_maxburst = p->fifosize / 4;
>  		data->data.dma.txconf.dst_maxburst = p->fifosize / 4;
> - 		up->dma = &data->data.dma;
> +		up->dma = 0; // Proof of concept, not to be merged!

I'll need to look into this.

> 2.- Although "linux,rs485-enabled-at-boot-time" is set in the DTS,
> the RTS/DriverEnable line is asserted all the time the /dev/ttyS1
> device file is closed.
> As soon as the device file is openned, the RTS line is deasserted.
> Then it works as expected: it is asserted only during transmissions.
> When the device file is closed again, the RTS line goes back to the
> asserted level and stays there.
> When the rs485 mode is enabled, it is expected that the RTS line be
> deasserted by default.

Managing RTS is a mess in 8250 driver as has recently being noted. It will 
hopefully get sorted out eventually.

> 3.- The RTS line is asserted a few microseconds earlier than the
> start bit, that is acceptable, but then it deasserts one whole bit
> time before the last stop bit.
> So, the last stop bit of the last byte of a message is not sent
> because the driver is disabled.
> This has been tested with the port configured at 19200e1, that is,
> the bit time is 52 us.
> I worked around this by adding "rs485-rts-delay = <0 52>;" in the
> DTS. This leads to the following feature (not an issue):
> 
> On Mon, 11 Apr 2022 11:33:12 +0300, Ilpo Järvinen wrote:
> > Set delay_rts_before_send and delay_rts_after_send to zero for now.
> > The granularity of that ABI is too coarse to be useful.
> 
> Indeed the time unit of this parameter is milliseconds, as stated in
> Documentation/devicetree/bindings/serial/rs485.yaml
> Which in the general case is more than ten bit times.
> 
> But it is being interpreted as microseconds here:
> 
> On Mon, 11 Apr 2022 11:33:11 +0300, Ilpo Järvinen wrote:
> > [PATCH v3 02/12] serial: 8250: Handle UART without interrupt on TEMT
> >+	stop_delay += (u64)p->port.rs485.delay_rts_after_send * NSEC_PER_USEC;
> 
> So, this way it has a useful granularity to be used in
> "rs485-rts-delay = <0 52>;" but is not compliant with the spec.

It seems I just got confused here with all these different time units. My 
intention was to use NSEC_PER_MSEC here to match the spec although it's 
not that useful granularity. I'll change that for the next version.
Lukas was planning of making it much finer granularity with nsecs (leaving 
the small values for msecs for compat purposes) which would hopefully 
resolve this granularity challenge.

About the actual issue of too early deassert. It kind of sounds like the 
stop tx timer was not waiting long enough. It could be that THRE is 
asserted sooner than I expected (maybe HW does FIFO->shift register
during the transmission of the stop bit of the prev char asserting THRE
approx one bit too early).

Perhaps this patch would help to combat the problem (roughly estimating
worst-case one bit time here with that /7):

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 35fbaa53bc2f..f944c639db82 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1551,7 +1551,7 @@ static inline void __stop_tx(struct uart_8250_port *p)
 		if (!(lsr & UART_LSR_TEMT)) {
 			if (!(p->capabilities & UART_CAP_NOTEMT))
 				return;
-			stop_delay = p->port.frame_time;
+			stop_delay = p->port.frame_time + DIV_ROUND_UP(p->port.frame_time, 7);
 		}
 
 		__stop_tx_rs485(p, stop_delay);