[PATCH] Revert "serial: max310x: rework RX interrupt handling"

[PATCH] Revert "serial: max310x: rework RX interrupt handling"

[Alexander Shiyan]

This reverts commit fce3c5c1a2d9cd888f2987662ce17c0c651916b2.

FIFO is triggered 4 intervals after receiving a byte, it's good
when we don't care about the time of reception, but are only
interested in the presence of any activity on the line.
Unfortunately, this method is not suitable for all tasks,
for example, the RS-485 protocol will not work properly,
since the state machine must track the request-response time
and after the timeout expires, a decision is made that the device
on the line is not responding.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/tty/serial/max310x.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 9795b2e8b0b2..1b61d26bb7af 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1056,9 +1056,9 @@ static int max310x_startup(struct uart_port *port)
 	max310x_port_update(port, MAX310X_MODE1_REG,
 			    MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
 
-	/* Reset FIFOs */
-	max310x_port_write(port, MAX310X_MODE2_REG,
-			   MAX310X_MODE2_FIFORST_BIT);
+	/* Configure MODE2 register & Reset FIFOs*/
+	val = MAX310X_MODE2_RXEMPTINV_BIT | MAX310X_MODE2_FIFORST_BIT;
+	max310x_port_write(port, MAX310X_MODE2_REG, val);
 	max310x_port_update(port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_FIFORST_BIT, 0);
 
@@ -1086,27 +1086,8 @@ static int max310x_startup(struct uart_port *port)
 	/* Clear IRQ status register */
 	max310x_port_read(port, MAX310X_IRQSTS_REG);
 
-	/*
-	 * Let's ask for an interrupt after a timeout equivalent to
-	 * the receiving time of 4 characters after the last character
-	 * has been received.
-	 */
-	max310x_port_write(port, MAX310X_RXTO_REG, 4);
-
-	/*
-	 * Make sure we also get RX interrupts when the RX FIFO is
-	 * filling up quickly, so get an interrupt when half of the RX
-	 * FIFO has been filled in.
-	 */
-	max310x_port_write(port, MAX310X_FIFOTRIGLVL_REG,
-			   MAX310X_FIFOTRIGLVL_RX(MAX310X_FIFO_SIZE / 2));
-
-	/* Enable RX timeout interrupt in LSR */
-	max310x_port_write(port, MAX310X_LSR_IRQEN_REG,
-			   MAX310X_LSR_RXTO_BIT);
-
-	/* Enable LSR, RX FIFO trigger, CTS change interrupts */
-	val = MAX310X_IRQ_LSR_BIT  | MAX310X_IRQ_RXFIFO_BIT | MAX310X_IRQ_TXEMPTY_BIT;
+	/* Enable RX, TX, CTS change interrupts */
+	val = MAX310X_IRQ_RXEMPTY_BIT | MAX310X_IRQ_TXEMPTY_BIT;
 	max310x_port_write(port, MAX310X_IRQEN_REG, val | MAX310X_IRQ_CTS_BIT);
 
 	return 0;
-- 
2.26.2

Re: [PATCH] Revert "serial: max310x: rework RX interrupt handling"

[Thomas Petazzoni]

Hello,

On Wed, 17 Feb 2021 11:06:08 +0300
Alexander Shiyan <shc_work@mail.ru> wrote:

> This reverts commit fce3c5c1a2d9cd888f2987662ce17c0c651916b2.
> 
> FIFO is triggered 4 intervals after receiving a byte, it's good
> when we don't care about the time of reception, but are only
> interested in the presence of any activity on the line.
> Unfortunately, this method is not suitable for all tasks,
> for example, the RS-485 protocol will not work properly,
> since the state machine must track the request-response time
> and after the timeout expires, a decision is made that the device
> on the line is not responding.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Thanks for the feedback. Should we switch between one mode and the
other depending on whether RS232 or RS485 is used ? Or is there some
appropriate user-space interface to ask the UART driver to tweak this
kind of configuration ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] Revert "serial: max310x: rework RX interrupt handling"

[Alexander Shiyan]

On Wed, 17 Feb 2021 23:53:45 +0100
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

Hello.

> > This reverts commit fce3c5c1a2d9cd888f2987662ce17c0c651916b2.
> > 
> > FIFO is triggered 4 intervals after receiving a byte, it's good
> > when we don't care about the time of reception, but are only
> > interested in the presence of any activity on the line.
> > Unfortunately, this method is not suitable for all tasks,
> > for example, the RS-485 protocol will not work properly,
> > since the state machine must track the request-response time
> > and after the timeout expires, a decision is made that the device
> > on the line is not responding.
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> 
> Thanks for the feedback. Should we switch between one mode and the
> other depending on whether RS232 or RS485 is used ? Or is there some
> appropriate user-space interface to ask the UART driver to tweak this
> kind of configuration ?

I wrote a little inaccurately, I did not mean a physical interface RS-485,
but rather a time-critical MODBUS protocol. (In our case it used on top on RS-485).

I do not know how best to solve this problem, it may be an additional parameter
for the devicetree, but in this case it is not clear how to manage it if the
devicetree is not used ...
This could be a Kconfig item (but something's not very good either).
Probably the best solution is to adapt some kind of IOCTL (or control via SYSFS).

In any case i feel free to test your modifications to the driver.

Thanks!
-- 
Alexander Shiyan <shc_work@mail.ru>

Re: [PATCH] Revert "serial: max310x: rework RX interrupt handling"

[Thomas Petazzoni]

Hello,

On Thu, 18 Feb 2021 06:55:53 +0300
Alexander Shiyan <shc_work@mail.ru> wrote:

> I wrote a little inaccurately, I did not mean a physical interface RS-485,
> but rather a time-critical MODBUS protocol. (In our case it used on top on RS-485).

OK, so it's not about 232 vs 485.

> I do not know how best to solve this problem, it may be an additional parameter
> for the devicetree, but in this case it is not clear how to manage it if the
> devicetree is not used ...

No, it cannot be a parameter in the Device Tree, as what we're talking
about is not hardware description, but configuration of the hardware
for particular use cases.

> This could be a Kconfig item (but something's not very good either).
> Probably the best solution is to adapt some kind of IOCTL (or control via SYSFS).

Greg, Jiri, perhaps you could comment on what would be the appropriate
user-space interface to use or add to be able to configure such aspects
of a UART controller ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] Revert "serial: max310x: rework RX interrupt handling"

[Greg Kroah-Hartman]

On Thu, Feb 18, 2021 at 07:51:27AM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 18 Feb 2021 06:55:53 +0300
> Alexander Shiyan <shc_work@mail.ru> wrote:
> 
> > I wrote a little inaccurately, I did not mean a physical interface RS-485,
> > but rather a time-critical MODBUS protocol. (In our case it used on top on RS-485).
> 
> OK, so it's not about 232 vs 485.
> 
> > I do not know how best to solve this problem, it may be an additional parameter
> > for the devicetree, but in this case it is not clear how to manage it if the
> > devicetree is not used ...
> 
> No, it cannot be a parameter in the Device Tree, as what we're talking
> about is not hardware description, but configuration of the hardware
> for particular use cases.
> 
> > This could be a Kconfig item (but something's not very good either).
> > Probably the best solution is to adapt some kind of IOCTL (or control via SYSFS).
> 
> Greg, Jiri, perhaps you could comment on what would be the appropriate
> user-space interface to use or add to be able to configure such aspects
> of a UART controller ?

What aspects need configuring and why is this uart so unique from all
others that it can't use the normal configuration methods?

thanks,

greg k-h

Re: [PATCH] Revert "serial: max310x: rework RX interrupt handling"

[Thomas Petazzoni]

Hello Greg,

On Thu, 18 Feb 2021 08:11:33 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> What aspects need configuring and why is this uart so unique from all
> others that it can't use the normal configuration methods?

What makes is stand out from other UARTs is that it is over SPI. So
accessing every single register of the UART is much more expensive than
with UART controllers on MMIO.

As explained in commit fce3c5c1a2d9cd888f2987662ce17c0c651916b2, in its
original state, the driver was configuring the UART to get an interrupt
for every single byte being received, causing a huge CPU load (25% in
my use case) for a very simple workload that consists in receiving 20
bytes every 16ms.

What fce3c5c1a2d9cd888f2987662ce17c0c651916b2 does it to configure the
UART controller to not interrupt at every byte being received, but only
when the RX FIFO has reached a certain threshold *or* after some time
without receiving data. Clearly, this is trading throughput against
latency.

So what needs to be configured are two aspects:

 * How many bytes the UART will receive before triggering an interrupt
   and delivering data to the TTY layer/userspace.

 * How much time with no data received before the UART triggers and
   interrupt, and received data is delivered to the TTY layer/userspace.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] Revert "serial: max310x: rework RX interrupt handling"

[Greg Kroah-Hartman]

On Thu, Feb 18, 2021 at 08:20:51AM +0100, Thomas Petazzoni wrote:
> Hello Greg,
> 
> On Thu, 18 Feb 2021 08:11:33 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > What aspects need configuring and why is this uart so unique from all
> > others that it can't use the normal configuration methods?
> 
> What makes is stand out from other UARTs is that it is over SPI. So
> accessing every single register of the UART is much more expensive than
> with UART controllers on MMIO.
> 
> As explained in commit fce3c5c1a2d9cd888f2987662ce17c0c651916b2, in its
> original state, the driver was configuring the UART to get an interrupt
> for every single byte being received, causing a huge CPU load (25% in
> my use case) for a very simple workload that consists in receiving 20
> bytes every 16ms.
> 
> What fce3c5c1a2d9cd888f2987662ce17c0c651916b2 does it to configure the
> UART controller to not interrupt at every byte being received, but only
> when the RX FIFO has reached a certain threshold *or* after some time
> without receiving data. Clearly, this is trading throughput against
> latency.
> 
> So what needs to be configured are two aspects:
> 
>  * How many bytes the UART will receive before triggering an interrupt
>    and delivering data to the TTY layer/userspace.

Odds are, this is already an ioctl for the tty layer given that this
seems like a common thing even for "normal" uarts.  Has anyone looked?

>  * How much time with no data received before the UART triggers and
>    interrupt, and received data is delivered to the TTY layer/userspace.

Again, that should already be an option we support, if not, I would be
surprised.  I don't have the time to dig through the
kernel/documentation at the moment, but if someone that cares about this
can, I would appreciate it :)

thanks,

greg k-h