serial: amba-pl011: Fix RTS handling in RS485 mode

[pelwell]

commit 2c1fd53 upstream.

Data loss on serial line was observed during communication through serial ports ttyAMA1 and ttyAMA2 interconnected via RS485 transcievers. Both ports are in one BCM2711 (Compute Module CM40) and they share the same interrupt line.

The problem is caused by long waiting for tx queue flush in the function pl011_rs485_tx_stop. Udelay or mdelay are used to wait. The function is called from the interrupt handler. If multiple devices share a single interrupt line, late processing of pending interrupts and data loss may occur. When operation of both devices are synchronous, collisions are quite often.

This rework is based on the method used in tty/serial/imx.c Use hrtimer instead of udelay and mdelay calls.
Replace simple bool variable rs485_tx_started by 4-state variable rs485_tx_state.

Tested-by: Lino Sanfilippo [email protected]

Link: https://lore.kernel.org/r/[email protected]

[nbuchwitz]

hrtimer_init(&uap->trigger_start_tx, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
hrtimer_init(&uap->trigger_stop_tx, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
uap->trigger_start_tx.function = pl011_trigger_start_tx;
uap->trigger_stop_tx.function = pl011_trigger_stop_tx;

is missing in pl011_axi_probe() in order to make this work with rp1

[pelwell]

Like this?

[nbuchwitz]

Yes, that's the same as what I tested here.

There is still a significant performance regression (essentially unusable) with our RS485-based backplane driver. I would really appreciate holding off on the merge until we have investigated this further. Otherwise, we would need to revert this downstream.

[pelwell]

It's going nowhere for now.

[nbuchwitz]

I've put together a workaround that brings back stable functionality for our serdev-based backplane driver:

https://gitlab.com/revolutionpi/linux/-/commit/c3cae2df785ea604b49addfd2f7d75159e91475a

Since we'd like to get as much as possible into upstream, I'm still figuring out how to frame this in a way that makes sense for mainline. Before posting to the mailing list, I wanted to check if you or Jonathan have any thoughts on this.
Any feedback would be much appreciated!

[nbuchwitz]

I recently learned that converting a PR to draft status disables notifications for subscribers. Could you confirm whether you received my earlier comment, @pelwell

[pelwell]

I recently learned that converting a PR to draft status disables notifications for subscribers.

Does it? That doesn't sound correct.

Yes, I did see the comment, but I was in the middle of something engrossing and haven't got back to it. I did take a look at the patch and thought it was not a simple "yes" (which is why you asked the question). But in practice, I trust that you wouldn't be proposing a complex solution if you knew of a simple one, and you've spent far longer looking at this problem than I have.

Turn your patch into a PR and we can encourage people to try it.

[6by9]

There are almost always compromises with RS485 transceivers on UARTS that give a FIFO empty rather than shift register empty interrupt.

The only solution I've seen reliably work for 2-wire RS485 without spinning is to stuff an extra byte into the FIFO when you hit the end of a message. Disable RTS when that extra byte is loaded into the shift register and therefore it is effectively discard it because the RS485 TX isn't active. (That worked on Z8530 and 8250 UARTs)
You would need to ensure that an extra data submitted to the UART during that dummy byte period wasn't immediately loaded into the FIFO and enabled RTS.

I haven't fully thought through whether that is another option here.

[nbuchwitz]

There are almost always compromises with RS485 transceivers on UARTS that give a FIFO empty rather than shift register empty interrupt.

The only solution I've seen reliably work for 2-wire RS485 without spinning is to stuff an extra byte into the FIFO when you hit the end of a message. Disable RTS when that extra byte is loaded into the shift register and therefore it is effectively discard it because the RS485 TX isn't active. (That worked on Z8530 and 8250 UARTs) You would need to ensure that an extra data submitted to the UART during that dummy byte period wasn't immediately loaded into the FIFO and enabled RTS.

I haven't fully thought through whether that is another option here.

Thanks! Bit of a bummer if we lose DMA - it finally works with RP1 and PL011. But yeah, worth exploring.

[pelwell]

Updated with @nbuchwitz's commit from #7221.