linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] serial: max310x: rework RX interrupt handling
@ 2020-10-01  7:44 Thomas Petazzoni
  2020-10-01 11:35 ` Andy Shevchenko
  2020-10-28 16:20 ` Jan Kundrát
  0 siblings, 2 replies; 5+ messages in thread
From: Thomas Petazzoni @ 2020-10-01  7:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: linux-serial, linux-kernel, Jan Kundrát, Mark Brown,
	Andy Shevchenko, linux-spi, Thomas Petazzoni

Currently, the RX interrupt logic uses the RXEMPTY interrupt, with the
RXEMPTYINV bit set, which means we get an RX interrupt as soon as the
RX FIFO is non-empty.

However, with the MAX310X having a FIFO of 128 bytes, this makes very
poor use of the FIFO: we trigger an interrupt as soon as the RX FIFO
has one byte, which means a lot of interrupts, each only collecting a
few bytes from the FIFO, causing a significant CPU load.

Instead this commit relies on two other RX interrupt events:

 - MAX310X_IRQ_RXFIFO_BIT, which triggers when the RX FIFO has reached
   a certain threshold, which we define to be half of the FIFO
   size. This ensure we get an interrupt before the RX FIFO fills up.

 - MAX310X_LSR_RXTO_BIT, which triggers when the RX FIFO has received
   some bytes, and then no more bytes are received for a certain
   time. Arbitrarily, this time is defined to the time is takes to
   receive 4 characters.

On a Microchip SAMA5D3 platform that is receiving 20 bytes every 16ms
over one MAX310X UART, this patch has allowed to reduce the CPU
consumption of the interrupt handler thread from ~25% to 6-7%.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
Changes since v1:
- Fix missing space when closing a comment
---
 drivers/tty/serial/max310x.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 8434bd5a8ec7..21130af106bb 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);
 
-	/* Configure MODE2 register & Reset FIFOs*/
-	val = MAX310X_MODE2_RXEMPTINV_BIT | MAX310X_MODE2_FIFORST_BIT;
-	max310x_port_write(port, MAX310X_MODE2_REG, val);
+	/* Reset FIFOs */
+	max310x_port_write(port, MAX310X_MODE2_REG,
+			   MAX310X_MODE2_FIFORST_BIT);
 	max310x_port_update(port, MAX310X_MODE2_REG,
 			    MAX310X_MODE2_FIFORST_BIT, 0);
 
@@ -1086,8 +1086,27 @@ static int max310x_startup(struct uart_port *port)
 	/* Clear IRQ status register */
 	max310x_port_read(port, MAX310X_IRQSTS_REG);
 
-	/* Enable RX, TX, CTS change interrupts */
-	val = MAX310X_IRQ_RXEMPTY_BIT | MAX310X_IRQ_TXEMPTY_BIT;
+	/*
+	 * 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;
 	max310x_port_write(port, MAX310X_IRQEN_REG, val | MAX310X_IRQ_CTS_BIT);
 
 	return 0;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] serial: max310x: rework RX interrupt handling
  2020-10-01  7:44 [PATCH v2] serial: max310x: rework RX interrupt handling Thomas Petazzoni
@ 2020-10-01 11:35 ` Andy Shevchenko
  2020-10-28 16:20 ` Jan Kundrát
  1 sibling, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2020-10-01 11:35 UTC (permalink / raw)
  To: Thomas Petazzoni
  Cc: Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List, Jan Kundrát, Mark Brown,
	linux-spi

On Thu, Oct 1, 2020 at 10:44 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Currently, the RX interrupt logic uses the RXEMPTY interrupt, with the
> RXEMPTYINV bit set, which means we get an RX interrupt as soon as the
> RX FIFO is non-empty.
>
> However, with the MAX310X having a FIFO of 128 bytes, this makes very
> poor use of the FIFO: we trigger an interrupt as soon as the RX FIFO
> has one byte, which means a lot of interrupts, each only collecting a
> few bytes from the FIFO, causing a significant CPU load.
>
> Instead this commit relies on two other RX interrupt events:
>
>  - MAX310X_IRQ_RXFIFO_BIT, which triggers when the RX FIFO has reached
>    a certain threshold, which we define to be half of the FIFO
>    size. This ensure we get an interrupt before the RX FIFO fills up.
>
>  - MAX310X_LSR_RXTO_BIT, which triggers when the RX FIFO has received
>    some bytes, and then no more bytes are received for a certain
>    time. Arbitrarily, this time is defined to the time is takes to
>    receive 4 characters.
>
> On a Microchip SAMA5D3 platform that is receiving 20 bytes every 16ms
> over one MAX310X UART, this patch has allowed to reduce the CPU
> consumption of the interrupt handler thread from ~25% to 6-7%.
>

You missed my tag :-(

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
> Changes since v1:
> - Fix missing space when closing a comment
> ---
>  drivers/tty/serial/max310x.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 8434bd5a8ec7..21130af106bb 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);
>
> -       /* Configure MODE2 register & Reset FIFOs*/
> -       val = MAX310X_MODE2_RXEMPTINV_BIT | MAX310X_MODE2_FIFORST_BIT;
> -       max310x_port_write(port, MAX310X_MODE2_REG, val);
> +       /* Reset FIFOs */
> +       max310x_port_write(port, MAX310X_MODE2_REG,
> +                          MAX310X_MODE2_FIFORST_BIT);
>         max310x_port_update(port, MAX310X_MODE2_REG,
>                             MAX310X_MODE2_FIFORST_BIT, 0);
>
> @@ -1086,8 +1086,27 @@ static int max310x_startup(struct uart_port *port)
>         /* Clear IRQ status register */
>         max310x_port_read(port, MAX310X_IRQSTS_REG);
>
> -       /* Enable RX, TX, CTS change interrupts */
> -       val = MAX310X_IRQ_RXEMPTY_BIT | MAX310X_IRQ_TXEMPTY_BIT;
> +       /*
> +        * 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;
>         max310x_port_write(port, MAX310X_IRQEN_REG, val | MAX310X_IRQ_CTS_BIT);
>
>         return 0;
> --
> 2.26.2
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] serial: max310x: rework RX interrupt handling
  2020-10-01  7:44 [PATCH v2] serial: max310x: rework RX interrupt handling Thomas Petazzoni
  2020-10-01 11:35 ` Andy Shevchenko
@ 2020-10-28 16:20 ` Jan Kundrát
  2020-10-28 17:06   ` Thomas Petazzoni
  1 sibling, 1 reply; 5+ messages in thread
From: Jan Kundrát @ 2020-10-28 16:20 UTC (permalink / raw)
  To: Thomas Petazzoni
  Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
	Mark Brown, Andy Shevchenko, linux-spi

On čtvrtek 1. října 2020 9:44:15 CEST, Thomas Petazzoni wrote:
> Currently, the RX interrupt logic uses the RXEMPTY interrupt, with the
> RXEMPTYINV bit set, which means we get an RX interrupt as soon as the
> RX FIFO is non-empty.
>
> However, with the MAX310X having a FIFO of 128 bytes, this makes very
> poor use of the FIFO: we trigger an interrupt as soon as the RX FIFO
> has one byte, which means a lot of interrupts, each only collecting a
> few bytes from the FIFO, causing a significant CPU load.

Thanks for taking the time to write this patch. We're using MAX14830 on a 
Clearfog Base board via a 26 MHz SPI bus. Our code polls a custom 
peripheral over UART at 115200 baud ten times a second; the messages are 
typically shorter than 50 chars. Before this patch, `perf top --sort 
comm,dso` showed about 28% CPU load for the corresponding SPI kthread, 
after applying this patch it's between 3 and 5%. That's cool :).

Tested-by: Jan Kundrát <jan.kundrat@cesnet.cz>
Reviewed-by: Jan Kundrát <jan.kundrat@cesnet.cz>

(but see below, please)

> +	/* Enable LSR, RX FIFO trigger, CTS change interrupts */
> +	val = MAX310X_IRQ_LSR_BIT  | MAX310X_IRQ_RXFIFO_BIT | 
> MAX310X_IRQ_TXEMPTY_BIT;
>  	max310x_port_write(port, MAX310X_IRQEN_REG, val | MAX310X_IRQ_CTS_BIT);

This comment doesn't fully match that code, and also the effective value 
that is written to the register is split into two statements. What about 
just:

+	/* Enable LSR, RX FIFO trigger, TX FIFO empty, CTS change interrupts */
+ 	max310x_port_write(port, MAX310X_IRQEN_REG, MAX310X_IRQ_LSR_BIT | 
MAX310X_IRQ_RXFIFO_BIT | MAX310X_IRQ_TXEMPTY_BIT | MAX310X_IRQ_CTS_BIT);

With kind regards,
Jan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] serial: max310x: rework RX interrupt handling
  2020-10-28 16:20 ` Jan Kundrát
@ 2020-10-28 17:06   ` Thomas Petazzoni
  2020-10-28 17:14     ` Thomas Petazzoni
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Petazzoni @ 2020-10-28 17:06 UTC (permalink / raw)
  To: Jan Kundrát
  Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
	Mark Brown, Andy Shevchenko, linux-spi

Hello,

On Wed, 28 Oct 2020 17:20:24 +0100
Jan Kundrát <jan.kundrat@cesnet.cz> wrote:

> Thanks for taking the time to write this patch. We're using MAX14830 on a 
> Clearfog Base board via a 26 MHz SPI bus. Our code polls a custom 
> peripheral over UART at 115200 baud ten times a second; the messages are 
> typically shorter than 50 chars. Before this patch, `perf top --sort 
> comm,dso` showed about 28% CPU load for the corresponding SPI kthread, 
> after applying this patch it's between 3 and 5%. That's cool :).
> 
> Tested-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> Reviewed-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> 
> (but see below, please)

Thanks for your review and testing, and glad to hear that it also
improves the CPU load on your use-case.

> > +	/* Enable LSR, RX FIFO trigger, CTS change interrupts */
> > +	val = MAX310X_IRQ_LSR_BIT  | MAX310X_IRQ_RXFIFO_BIT | 
> > MAX310X_IRQ_TXEMPTY_BIT;
> >  	max310x_port_write(port, MAX310X_IRQEN_REG, val | MAX310X_IRQ_CTS_BIT);  
> 
> This comment doesn't fully match that code, and also the effective value 
> that is written to the register is split into two statements. What about 
> just:
> 
> +	/* Enable LSR, RX FIFO trigger, TX FIFO empty, CTS change interrupts */
> + 	max310x_port_write(port, MAX310X_IRQEN_REG, MAX310X_IRQ_LSR_BIT | 
> MAX310X_IRQ_RXFIFO_BIT | MAX310X_IRQ_TXEMPTY_BIT | MAX310X_IRQ_CTS_BIT);

Indeed, the comment should be updated, I'll fix that. Regarding the
effective value computed in two steps, it was already the case in the
current code:

	/* 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);

but granted, that's not an excuse not to fix it.

On my way to send a v3 :-)

Thanks again,

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] serial: max310x: rework RX interrupt handling
  2020-10-28 17:06   ` Thomas Petazzoni
@ 2020-10-28 17:14     ` Thomas Petazzoni
  0 siblings, 0 replies; 5+ messages in thread
From: Thomas Petazzoni @ 2020-10-28 17:14 UTC (permalink / raw)
  To: Jan Kundrát
  Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, linux-kernel,
	Mark Brown, Andy Shevchenko, linux-spi

Hello,

On Wed, 28 Oct 2020 18:06:51 +0100
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On my way to send a v3 :-)

Well, in fact the patch is upstream already:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/tty/serial/max310x.c?id=fce3c5c1a2d9cd888f2987662ce17c0c651916b2

So I'll send a follow-up patch instead.

Thanks,

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-10-28 22:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-01  7:44 [PATCH v2] serial: max310x: rework RX interrupt handling Thomas Petazzoni
2020-10-01 11:35 ` Andy Shevchenko
2020-10-28 16:20 ` Jan Kundrát
2020-10-28 17:06   ` Thomas Petazzoni
2020-10-28 17:14     ` Thomas Petazzoni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).