Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] serial: max310x: rework RX interrupt handling
@ 2020-09-30 13:51 Thomas Petazzoni
  2020-09-30 14:24 ` Andy Shevchenko
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Petazzoni @ 2020-09-30 13:51 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>
---
 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 8434bd5a8ec78..a1c80850d77ed 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	[flat|nested] 5+ messages in thread

* Re: [PATCH] serial: max310x: rework RX interrupt handling
  2020-09-30 13:51 [PATCH] serial: max310x: rework RX interrupt handling Thomas Petazzoni
@ 2020-09-30 14:24 ` Andy Shevchenko
  2020-09-30 14:50   ` Thomas Petazzoni
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2020-09-30 14:24 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 Wed, Sep 30, 2020 at 4:51 PM 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%.

Was it always like this?
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
See one nit-pick below.

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> ---
>  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 8434bd5a8ec78..a1c80850d77ed 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*/

Add a space before the asterisk.

> +       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] serial: max310x: rework RX interrupt handling
  2020-09-30 14:24 ` Andy Shevchenko
@ 2020-09-30 14:50   ` Thomas Petazzoni
  2020-09-30 14:54     ` Andy Shevchenko
  0 siblings, 1 reply; 5+ messages in thread
From: Thomas Petazzoni @ 2020-09-30 14:50 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List, Jan Kundrát, Mark Brown,
	linux-spi

Hello,

On Wed, 30 Sep 2020 17:24:48 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> > 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%.  
> 
> Was it always like this?

As far as I can see, yes. I guess you were thinking about a Fixes: tag
perhaps? I don't think a Fixes: tag really applies here, as we're not
fixing a performance regression.

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> > -       /* Configure MODE2 register & Reset FIFOs*/
> > -       val = MAX310X_MODE2_RXEMPTINV_BIT | MAX310X_MODE2_FIFORST_BIT;
> > -       max310x_port_write(port, MAX310X_MODE2_REG, val);
> > +       /* Reset FIFOs*/  
> 
> Add a space before the asterisk.

Indeed. Let me know if a v2 is needed, or if this small typo can be
fixed when applying.

Thanks,

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

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

* Re: [PATCH] serial: max310x: rework RX interrupt handling
  2020-09-30 14:50   ` Thomas Petazzoni
@ 2020-09-30 14:54     ` Andy Shevchenko
  2020-10-02 12:40       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2020-09-30 14:54 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 Wed, Sep 30, 2020 at 5:50 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
> On Wed, 30 Sep 2020 17:24:48 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> > > 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%.
> >
> > Was it always like this?
>
> As far as I can see, yes. I guess you were thinking about a Fixes: tag
> perhaps? I don't think a Fixes: tag really applies here, as we're not
> fixing a performance regression.

Thanks for elaboration.

...

> > > +       /* Reset FIFOs*/
> >
> > Add a space before the asterisk.
>
> Indeed. Let me know if a v2 is needed, or if this small typo can be
> fixed when applying.

Greg usually doesn't touch commits, so v2 would be better.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH] serial: max310x: rework RX interrupt handling
  2020-09-30 14:54     ` Andy Shevchenko
@ 2020-10-02 12:40       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2020-10-02 12:40 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Thomas Petazzoni, Jiri Slaby, open list:SERIAL DRIVERS,
	Linux Kernel Mailing List, Jan Kundrát, Mark Brown,
	linux-spi

On Wed, Sep 30, 2020 at 05:54:15PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 30, 2020 at 5:50 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
> > On Wed, 30 Sep 2020 17:24:48 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > > > 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%.
> > >
> > > Was it always like this?
> >
> > As far as I can see, yes. I guess you were thinking about a Fixes: tag
> > perhaps? I don't think a Fixes: tag really applies here, as we're not
> > fixing a performance regression.
> 
> Thanks for elaboration.
> 
> ...
> 
> > > > +       /* Reset FIFOs*/
> > >
> > > Add a space before the asterisk.
> >
> > Indeed. Let me know if a v2 is needed, or if this small typo can be
> > fixed when applying.
> 
> Greg usually doesn't touch commits, so v2 would be better.

Agreed, I don't like to edit things :)

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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 13:51 [PATCH] serial: max310x: rework RX interrupt handling Thomas Petazzoni
2020-09-30 14:24 ` Andy Shevchenko
2020-09-30 14:50   ` Thomas Petazzoni
2020-09-30 14:54     ` Andy Shevchenko
2020-10-02 12:40       ` Greg Kroah-Hartman

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git