[PATCH 0/8] serial: imx: work-around for hardware RX flood, and then isr improvements

[PATCH 0/8] serial: imx: work-around for hardware RX flood, and then isr improvements

[Sergey Organov]

These series contain a work-around for hardware RX flood in first 2
commits, and then further cleanups and optimizations of the receive IRQ
handler.

WARNING: the flood fix is extensively tested with older version of the
kernel with DMA turned off. The DMA path is tested only a bit, as DMA has
receive problems on my kernel version.

Sergey Organov (8):
  serial: imx: factor-out common code to imx_uart_soft_reset()
  serial: imx: work-around for hardware RX flood
  serial: imx: do not sysrq broken chars
  serial: imx: do not break from FIFO reading loop prematurely
  serial: imx: remove redundant USR2 read from FIFO reading loop
  serial: imx: stop using USR2 in FIFO reading loop
  serial: imx: use readl() to optimize FIFO reading loop
  serial: imx: refine local variables in rxint()

 drivers/tty/serial/imx.c | 227 ++++++++++++++++++++++++---------------
 1 file changed, 141 insertions(+), 86 deletions(-)

-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/8] serial: imx: factor-out common code to imx_uart_soft_reset()

[Sergey Organov]

We perform soft reset in 2 places, slightly differently for no sufficient
reasons, so move more generic variant to a function, and re-use the code.

Out of 2 repeat counters, 10 and 100, select 10, as the code works at
interrupts disabled, and in practice the reset happens immediately.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 73 ++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 757825edb0cd..bf222d8568a9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -397,6 +397,39 @@ static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
        hrtimer_start(hrt, ms_to_ktime(msec), HRTIMER_MODE_REL);
 }
 
+/* called with port.lock taken and irqs off */
+static void imx_uart_soft_reset(struct imx_port *sport)
+{
+	int i = 10;
+	u32 ucr2, ubir, ubmr, uts;
+
+	/*
+	 * According to the Reference Manual description of the UART SRST bit:
+	 *
+	 * "Reset the transmit and receive state machines,
+	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
+	 * and UTS[6-3]".
+	 *
+	 * We don't need to restore the old values from USR1, USR2, URXD and
+	 * UTXD. UBRC is read only, so only save/restore the other three
+	 * registers.
+	 */
+	ubir = imx_uart_readl(sport, UBIR);
+	ubmr = imx_uart_readl(sport, UBMR);
+	uts = imx_uart_readl(sport, IMX21_UTS);
+
+	ucr2 = imx_uart_readl(sport, UCR2);
+	imx_uart_writel(sport, ucr2 & ~UCR2_SRST, UCR2);
+
+	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
+		udelay(1);
+
+	/* Restore the registers */
+	imx_uart_writel(sport, ubir, UBIR);
+	imx_uart_writel(sport, ubmr, UBMR);
+	imx_uart_writel(sport, uts, IMX21_UTS);
+}
+
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1398,7 +1431,7 @@ static void imx_uart_disable_dma(struct imx_port *sport)
 static int imx_uart_startup(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
-	int retval, i;
+	int retval;
 	unsigned long flags;
 	int dma_is_inited = 0;
 	u32 ucr1, ucr2, ucr3, ucr4, uts;
@@ -1430,15 +1463,9 @@ static int imx_uart_startup(struct uart_port *port)
 		dma_is_inited = 1;
 
 	spin_lock_irqsave(&sport->port.lock, flags);
+
 	/* Reset fifo's and state machines */
-	i = 100;
-
-	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~UCR2_SRST;
-	imx_uart_writel(sport, ucr2, UCR2);
-
-	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
-		udelay(1);
+	imx_uart_soft_reset(sport);
 
 	/*
 	 * Finally, clear and enable interrupts
@@ -1593,8 +1620,6 @@ static void imx_uart_flush_buffer(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	struct scatterlist *sgl = &sport->tx_sgl[0];
-	u32 ucr2;
-	int i = 100, ubir, ubmr, uts;
 
 	if (!sport->dma_chan_tx)
 		return;
@@ -1612,32 +1637,8 @@ static void imx_uart_flush_buffer(struct uart_port *port)
 		sport->dma_is_txing = 0;
 	}
 
-	/*
-	 * According to the Reference Manual description of the UART SRST bit:
-	 *
-	 * "Reset the transmit and receive state machines,
-	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
-	 * and UTS[6-3]".
-	 *
-	 * We don't need to restore the old values from USR1, USR2, URXD and
-	 * UTXD. UBRC is read only, so only save/restore the other three
-	 * registers.
-	 */
-	ubir = imx_uart_readl(sport, UBIR);
-	ubmr = imx_uart_readl(sport, UBMR);
-	uts = imx_uart_readl(sport, IMX21_UTS);
+	imx_uart_soft_reset(sport);
 
-	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~UCR2_SRST;
-	imx_uart_writel(sport, ucr2, UCR2);
-
-	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
-		udelay(1);
-
-	/* Restore the registers */
-	imx_uart_writel(sport, ubir, UBIR);
-	imx_uart_writel(sport, ubmr, UBMR);
-	imx_uart_writel(sport, uts, IMX21_UTS);
 }
 
 static void
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] serial: imx: factor-out common code to imx_uart_soft_reset()

[Ilpo Järvinen]

On Fri, 13 Jan 2023, Sergey Organov wrote:

> We perform soft reset in 2 places, slightly differently for no sufficient
> reasons, so move more generic variant to a function, and re-use the code.
> 
> Out of 2 repeat counters, 10 and 100, select 10, as the code works at
> interrupts disabled, and in practice the reset happens immediately.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 73 ++++++++++++++++++++--------------------
>  1 file changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 757825edb0cd..bf222d8568a9 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -397,6 +397,39 @@ static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
>         hrtimer_start(hrt, ms_to_ktime(msec), HRTIMER_MODE_REL);
>  }
>  
> +/* called with port.lock taken and irqs off */
> +static void imx_uart_soft_reset(struct imx_port *sport)
> +{
> +	int i = 10;
> +	u32 ucr2, ubir, ubmr, uts;
> +
> +	/*
> +	 * According to the Reference Manual description of the UART SRST bit:
> +	 *
> +	 * "Reset the transmit and receive state machines,
> +	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
> +	 * and UTS[6-3]".
> +	 *
> +	 * We don't need to restore the old values from USR1, USR2, URXD and
> +	 * UTXD. UBRC is read only, so only save/restore the other three
> +	 * registers.
> +	 */
> +	ubir = imx_uart_readl(sport, UBIR);
> +	ubmr = imx_uart_readl(sport, UBMR);
> +	uts = imx_uart_readl(sport, IMX21_UTS);
> +
> +	ucr2 = imx_uart_readl(sport, UCR2);
> +	imx_uart_writel(sport, ucr2 & ~UCR2_SRST, UCR2);
> +
> +	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
> +		udelay(1);

This could use read_poll_timeout_atomic().

-- 
 i.


> +	/* Restore the registers */
> +	imx_uart_writel(sport, ubir, UBIR);
> +	imx_uart_writel(sport, ubmr, UBMR);
> +	imx_uart_writel(sport, uts, IMX21_UTS);
> +}
> +
>  /* called with port.lock taken and irqs off */
>  static void imx_uart_start_rx(struct uart_port *port)
>  {
> @@ -1398,7 +1431,7 @@ static void imx_uart_disable_dma(struct imx_port *sport)
>  static int imx_uart_startup(struct uart_port *port)
>  {
>  	struct imx_port *sport = (struct imx_port *)port;
> -	int retval, i;
> +	int retval;
>  	unsigned long flags;
>  	int dma_is_inited = 0;
>  	u32 ucr1, ucr2, ucr3, ucr4, uts;
> @@ -1430,15 +1463,9 @@ static int imx_uart_startup(struct uart_port *port)
>  		dma_is_inited = 1;
>  
>  	spin_lock_irqsave(&sport->port.lock, flags);
> +
>  	/* Reset fifo's and state machines */
> -	i = 100;
> -
> -	ucr2 = imx_uart_readl(sport, UCR2);
> -	ucr2 &= ~UCR2_SRST;
> -	imx_uart_writel(sport, ucr2, UCR2);
> -
> -	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
> -		udelay(1);
> +	imx_uart_soft_reset(sport);
>  
>  	/*
>  	 * Finally, clear and enable interrupts
> @@ -1593,8 +1620,6 @@ static void imx_uart_flush_buffer(struct uart_port *port)
>  {
>  	struct imx_port *sport = (struct imx_port *)port;
>  	struct scatterlist *sgl = &sport->tx_sgl[0];
> -	u32 ucr2;
> -	int i = 100, ubir, ubmr, uts;
>  
>  	if (!sport->dma_chan_tx)
>  		return;
> @@ -1612,32 +1637,8 @@ static void imx_uart_flush_buffer(struct uart_port *port)
>  		sport->dma_is_txing = 0;
>  	}
>  
> -	/*
> -	 * According to the Reference Manual description of the UART SRST bit:
> -	 *
> -	 * "Reset the transmit and receive state machines,
> -	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
> -	 * and UTS[6-3]".
> -	 *
> -	 * We don't need to restore the old values from USR1, USR2, URXD and
> -	 * UTXD. UBRC is read only, so only save/restore the other three
> -	 * registers.
> -	 */
> -	ubir = imx_uart_readl(sport, UBIR);
> -	ubmr = imx_uart_readl(sport, UBMR);
> -	uts = imx_uart_readl(sport, IMX21_UTS);
> +	imx_uart_soft_reset(sport);
>  
> -	ucr2 = imx_uart_readl(sport, UCR2);
> -	ucr2 &= ~UCR2_SRST;
> -	imx_uart_writel(sport, ucr2, UCR2);
> -
> -	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
> -		udelay(1);
> -
> -	/* Restore the registers */
> -	imx_uart_writel(sport, ubir, UBIR);
> -	imx_uart_writel(sport, ubmr, UBMR);
> -	imx_uart_writel(sport, uts, IMX21_UTS);
>  }
>  
>  static void
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] serial: imx: factor-out common code to imx_uart_soft_reset()

[Sergey Organov]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> On Fri, 13 Jan 2023, Sergey Organov wrote:
>
>> We perform soft reset in 2 places, slightly differently for no sufficient
>> reasons, so move more generic variant to a function, and re-use the code.
>> 
>> Out of 2 repeat counters, 10 and 100, select 10, as the code works at
>> interrupts disabled, and in practice the reset happens immediately.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 73 ++++++++++++++++++++--------------------
>>  1 file changed, 37 insertions(+), 36 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index 757825edb0cd..bf222d8568a9 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -397,6 +397,39 @@ static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
>>         hrtimer_start(hrt, ms_to_ktime(msec), HRTIMER_MODE_REL);
>>  }
>>  
>> +/* called with port.lock taken and irqs off */
>> +static void imx_uart_soft_reset(struct imx_port *sport)
>> +{
>> +	int i = 10;
>> +	u32 ucr2, ubir, ubmr, uts;
>> +
>> +	/*
>> +	 * According to the Reference Manual description of the UART SRST bit:
>> +	 *
>> +	 * "Reset the transmit and receive state machines,
>> +	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
>> +	 * and UTS[6-3]".
>> +	 *
>> +	 * We don't need to restore the old values from USR1, USR2, URXD and
>> +	 * UTXD. UBRC is read only, so only save/restore the other three
>> +	 * registers.
>> +	 */
>> +	ubir = imx_uart_readl(sport, UBIR);
>> +	ubmr = imx_uart_readl(sport, UBMR);
>> +	uts = imx_uart_readl(sport, IMX21_UTS);
>> +
>> +	ucr2 = imx_uart_readl(sport, UCR2);
>> +	imx_uart_writel(sport, ucr2 & ~UCR2_SRST, UCR2);
>> +
>> +	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
>> +		udelay(1);
>
> This could use read_poll_timeout_atomic().

As this is just a factor-out of existing code that uses the loop, I'm not
sure if it's a good idea to change this along the way.

Do you want me to add yet another patch to the series? I'd rather not,
as I won't be able to test it on my outdated kernel anyway.

Thanks,
-- Sergey Organov

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] serial: imx: work-around for hardware RX flood

[Sergey Organov]

Check if hardware Rx flood is in progress, and issue soft reset to UART to
stop the flood.

A way to reproduce the flood (checked on iMX6SX) is: open iMX UART at 9600
8N1, and from external source send 0xf0 char at 115200 8N1. In about 90% of
cases this starts a flood of "receiving" of 0xff characters by the iMX UART
that is terminated by any activity on RxD line, or could be stopped by
issuing soft reset to the UART (just stop/start of RX does not help). Note
that in essence what we did here is sending isolated start bit about 2.4
times shorter than it is to be if issued on the UART configured baud rate.

There was earlier attempt to fix similar issue in: 'commit
b38cb7d25711 ("serial: imx: Disable new features of autobaud detection")',
but apparently it only gets harder to reproduce the issue after that
commit.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 123 ++++++++++++++++++++++++++++++---------
 1 file changed, 95 insertions(+), 28 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index bf222d8568a9..e7fce31e460d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -210,6 +210,9 @@ struct imx_port {
 
 	struct mctrl_gpios *gpios;
 
+	/* counter to stop 0xff flood */
+	int idle_counter;
+
 	/* shadow registers */
 	unsigned int ucr1;
 	unsigned int ucr2;
@@ -428,6 +431,8 @@ static void imx_uart_soft_reset(struct imx_port *sport)
 	imx_uart_writel(sport, ubir, UBIR);
 	imx_uart_writel(sport, ubmr, UBMR);
 	imx_uart_writel(sport, uts, IMX21_UTS);
+
+	sport->idle_counter = 0;
 }
 
 /* called with port.lock taken and irqs off */
@@ -834,15 +839,66 @@ static irqreturn_t imx_uart_txint(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/* Check if hardware Rx flood is in progress, and issue soft reset to stop it.
+ * This is to be called from Rx ISRs only when some bytes were actually
+ * received.
+ *
+ * A way to reproduce the flood (checked on iMX6SX) is: open iMX UART at 9600
+ * 8N1, and from external source send 0xf0 char at 115200 8N1. In about 90% of
+ * cases this starts a flood of "receiving" of 0xff characters by the iMX6 UART
+ * that is terminated by any activity on RxD line, or could be stopped by
+ * issuing soft reset to the UART (just stop/start of RX does not help). Note
+ * that what we do here is sending isolated start bit about 2.4 times shorter
+ * than it is to be on UART configured baud rate.
+ */
+static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
+{
+	/* To detect hardware 0xff flood we monitor RxD line between RX
+	 * interrupts to isolate "receiving" of char(s) with no activity
+	 * on RxD line, that'd never happen on actual data transfers.
+	 *
+	 * We use USR2_WAKE bit to check for activity on RxD line, but we have a
+	 * race here if we clear USR2_WAKE when receiving of a char is in
+	 * progress, so we might get RX interrupt later with USR2_WAKE bit
+	 * cleared. Note though that as we don't try to clear USR2_WAKE when we
+	 * detected no activity, this race may hide actual activity only once.
+	 *
+	 * Yet another case where receive interrupt may occur without RxD
+	 * activity is expiration of aging timer, so we consider this as well.
+	 *
+	 * We use 'idle_counter' to ensure that we got at least so many RX
+	 * interrupts without any detected activity on RxD line. 2 cases
+	 * described plus 1 to be on the safe side gives us a margin of 3,
+	 * below. In practice I was not able to produce a false positive to
+	 * induce soft reset at regular data transfers even using 1 as the
+	 * margin, so 3 is actually very strong.
+	 *
+	 * We count interrupts, not chars in 'idle-counter' for simplicity.
+	 */
+
+	if (usr2 & USR2_WAKE) {
+		imx_uart_writel(sport, USR2_WAKE, USR2);
+		sport->idle_counter = 0;
+	} else if (++sport->idle_counter > 3) {
+		dev_warn(sport->port.dev, "RX flood detected: soft reset.");
+		imx_uart_soft_reset(sport); /* also clears 'sport->idle_counter' */
+	}
+}
+
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
 	unsigned int rx, flg, ignored = 0;
 	struct tty_port *port = &sport->port.state->port;
+	u32 usr2;
 
-	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
-		u32 usr2;
+	usr2 = imx_uart_readl(sport, USR2);
 
+	/* If we received something, check for 0xff flood */
+	if (usr2 & USR2_RDR)
+		imx_uart_check_flood(sport, usr2);
+
+	for ( ; usr2 & USR2_RDR; usr2 = imx_uart_readl(sport, USR2)) {
 		flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
@@ -1180,55 +1236,64 @@ static void imx_uart_dma_rx_callback(void *data)
 	status = dmaengine_tx_status(chan, sport->rx_cookie, &state);
 
 	if (status == DMA_ERROR) {
+		spin_lock(&sport->port.lock);
 		imx_uart_clear_rx_errors(sport);
+		spin_unlock(&sport->port.lock);
 		return;
 	}
 
-	if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
+	/*
+	 * The state-residue variable represents the empty space
+	 * relative to the entire buffer. Taking this in consideration
+	 * the head is always calculated base on the buffer total
+	 * length - DMA transaction residue. The UART script from the
+	 * SDMA firmware will jump to the next buffer descriptor,
+	 * once a DMA transaction if finalized (IMX53 RM - A.4.1.2.4).
+	 * Taking this in consideration the tail is always at the
+	 * beginning of the buffer descriptor that contains the head.
+	 */
 
-		/*
-		 * The state-residue variable represents the empty space
-		 * relative to the entire buffer. Taking this in consideration
-		 * the head is always calculated base on the buffer total
-		 * length - DMA transaction residue. The UART script from the
-		 * SDMA firmware will jump to the next buffer descriptor,
-		 * once a DMA transaction if finalized (IMX53 RM - A.4.1.2.4).
-		 * Taking this in consideration the tail is always at the
-		 * beginning of the buffer descriptor that contains the head.
-		 */
+	/* Calculate the head */
+	rx_ring->head = sg_dma_len(sgl) - state.residue;
 
-		/* Calculate the head */
-		rx_ring->head = sg_dma_len(sgl) - state.residue;
+	/* Calculate the tail. */
+	bd_size = sg_dma_len(sgl) / sport->rx_periods;
+	rx_ring->tail = ((rx_ring->head-1) / bd_size) * bd_size;
 
-		/* Calculate the tail. */
-		bd_size = sg_dma_len(sgl) / sport->rx_periods;
-		rx_ring->tail = ((rx_ring->head-1) / bd_size) * bd_size;
+	if (rx_ring->head <= sg_dma_len(sgl) &&
+	    rx_ring->head > rx_ring->tail) {
 
-		if (rx_ring->head <= sg_dma_len(sgl) &&
-		    rx_ring->head > rx_ring->tail) {
+		/* Move data from tail to head */
+		r_bytes = rx_ring->head - rx_ring->tail;
 
-			/* Move data from tail to head */
-			r_bytes = rx_ring->head - rx_ring->tail;
+		/* If we received something, check for 0xff flood */
+		if (r_bytes > 0) {
+			spin_lock(&sport->port.lock);
+			imx_uart_check_flood(sport, imx_uart_readl(sport, USR2));
+			spin_unlock(&sport->port.lock);
+		}
+
+		if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
 
 			/* CPU claims ownership of RX DMA buffer */
 			dma_sync_sg_for_cpu(sport->port.dev, sgl, 1,
-				DMA_FROM_DEVICE);
+					    DMA_FROM_DEVICE);
 
 			w_bytes = tty_insert_flip_string(port,
-				sport->rx_buf + rx_ring->tail, r_bytes);
+							 sport->rx_buf + rx_ring->tail, r_bytes);
 
 			/* UART retrieves ownership of RX DMA buffer */
 			dma_sync_sg_for_device(sport->port.dev, sgl, 1,
-				DMA_FROM_DEVICE);
+					       DMA_FROM_DEVICE);
 
 			if (w_bytes != r_bytes)
 				sport->port.icount.buf_overrun++;
 
 			sport->port.icount.rx += w_bytes;
-		} else	{
-			WARN_ON(rx_ring->head > sg_dma_len(sgl));
-			WARN_ON(rx_ring->head <= rx_ring->tail);
 		}
+	} else	{
+		WARN_ON(rx_ring->head > sg_dma_len(sgl));
+		WARN_ON(rx_ring->head <= rx_ring->tail);
 	}
 
 	if (w_bytes) {
@@ -1304,6 +1369,8 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport)
 		imx_uart_writel(sport, USR2_ORE, USR2);
 	}
 
+	sport->idle_counter = 0;
+
 }
 
 #define TXTL_DEFAULT 2 /* reset default */
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] serial: imx: do not sysrq broken chars

[Sergey Organov]

Do not call uart_handle_sysrq_char() if we got any receive error along with
the character, as we don't want random junk to be considered a sysrq.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e7fce31e460d..1c950112a598 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -911,9 +911,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 				continue;
 		}
 
-		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
-			continue;
-
 		if (unlikely(rx & URXD_ERR)) {
 			if (rx & URXD_BRK)
 				sport->port.icount.brk++;
@@ -942,7 +939,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 				flg = TTY_OVERRUN;
 
 			sport->port.sysrq = 0;
-		}
+		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
+			continue;
 
 		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
 			goto out;
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] serial: imx: do not sysrq broken chars

[Ilpo Järvinen]

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

On Fri, 13 Jan 2023, Sergey Organov wrote:

> Do not call uart_handle_sysrq_char() if we got any receive error along with
> the character, as we don't want random junk to be considered a sysrq.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index e7fce31e460d..1c950112a598 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -911,9 +911,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  				continue;
>  		}
>  
> -		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> -			continue;
> -
>  		if (unlikely(rx & URXD_ERR)) {
>  			if (rx & URXD_BRK)
>  				sport->port.icount.brk++;
> @@ -942,7 +939,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  				flg = TTY_OVERRUN;
>  
>  			sport->port.sysrq = 0;
> -		}
> +		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> +			continue;
>  
>  		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
>  			goto out;

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>


-- 
 i.

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] serial: imx: do not sysrq broken chars

[Johan Hovold]

On Fri, Jan 13, 2023 at 09:43:29PM +0300, Sergey Organov wrote:
> Do not call uart_handle_sysrq_char() if we got any receive error along with
> the character, as we don't want random junk to be considered a sysrq.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index e7fce31e460d..1c950112a598 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -911,9 +911,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  				continue;
>  		}
>  
> -		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> -			continue;
> -
>  		if (unlikely(rx & URXD_ERR)) {
>  			if (rx & URXD_BRK)
>  				sport->port.icount.brk++;
> @@ -942,7 +939,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  				flg = TTY_OVERRUN;
>  
>  			sport->port.sysrq = 0;
> -		}
> +		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> +			continue;

Nit: missing braces {}

Note that you could also place just place this after the block due to
the reset of the sysrq time stamp.

>  
>  		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
>  			goto out;

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] serial: imx: do not sysrq broken chars

[Sergey Organov]

Johan Hovold <johan@kernel.org> writes:

> On Fri, Jan 13, 2023 at 09:43:29PM +0300, Sergey Organov wrote:
>> Do not call uart_handle_sysrq_char() if we got any receive error along with
>> the character, as we don't want random junk to be considered a sysrq.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index e7fce31e460d..1c950112a598 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -911,9 +911,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>>  				continue;
>>  		}
>>  
>> -		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
>> -			continue;
>> -
>>  		if (unlikely(rx & URXD_ERR)) {
>>  			if (rx & URXD_BRK)
>>  				sport->port.icount.brk++;
>> @@ -942,7 +939,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>>  				flg = TTY_OVERRUN;
>>  
>>  			sport->port.sysrq = 0;
>> -		}
>> +		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
>> +			continue;
>
> Nit: missing braces {}
>
> Note that you could also place just place this after the block due to
> the reset of the sysrq time stamp.

Thanks, I think I'll opt for adding braces. Relying on the reset of the
timestamp feels a bit convoluted.

-- 
Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] serial: imx: do not sysrq broken chars

[Johan Hovold]

On Tue, Jan 17, 2023 at 08:35:48PM +0300, Sergey Organov wrote:
> Johan Hovold <johan@kernel.org> writes:
> > On Fri, Jan 13, 2023 at 09:43:29PM +0300, Sergey Organov wrote:

> >> @@ -911,9 +911,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> >>  				continue;
> >>  		}
> >>  
> >> -		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> >> -			continue;
> >> -
> >>  		if (unlikely(rx & URXD_ERR)) {
> >>  			if (rx & URXD_BRK)
> >>  				sport->port.icount.brk++;
> >> @@ -942,7 +939,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> >>  				flg = TTY_OVERRUN;
> >>  
> >>  			sport->port.sysrq = 0;
> >> -		}
> >> +		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> >> +			continue;
> >
> > Nit: missing braces {}
> >
> > Note that you could also place just place this after the block due to
> > the reset of the sysrq time stamp.
> 
> Thanks, I think I'll opt for adding braces. Relying on the reset of the
> timestamp feels a bit convoluted.

I agree, it may be a bit too subtle.

Johan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] serial: imx: do not break from FIFO reading loop prematurely

[Sergey Organov]

There is no reason to prematurely break out of FIFO reading loop, and it
might cause needless reenters into ISR, so keep reading until FIFO is
empty.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 1c950112a598..b96b0edc7854 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -888,7 +888,7 @@ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
-	unsigned int rx, flg, ignored = 0;
+	unsigned int rx, flg;
 	struct tty_port *port = &sport->port.state->port;
 	u32 usr2;
 
@@ -921,11 +921,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 			if (rx & URXD_OVRRUN)
 				sport->port.icount.overrun++;
 
-			if (rx & sport->port.ignore_status_mask) {
-				if (++ignored > 100)
-					goto out;
+			if (rx & sport->port.ignore_status_mask)
 				continue;
-			}
 
 			rx &= (sport->port.read_status_mask | 0xFF);
 
@@ -943,13 +940,12 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 			continue;
 
 		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
-			goto out;
+			continue;
 
 		if (tty_insert_flip_char(port, rx, flg) == 0)
 			sport->port.icount.buf_overrun++;
 	}
 
-out:
 	tty_flip_buffer_push(port);
 
 	return IRQ_HANDLED;
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/8] serial: imx: remove redundant USR2 read from FIFO reading loop

[Sergey Organov]

There is no need to read USR2 twice at every loop iteration: get rid of the
second read.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index b96b0edc7854..c44a7293c013 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -904,7 +904,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 
 		rx = imx_uart_readl(sport, URXD0);
 
-		usr2 = imx_uart_readl(sport, USR2);
 		if (usr2 & USR2_BRCD) {
 			imx_uart_writel(sport, USR2_BRCD, USR2);
 			if (uart_handle_break(&sport->port))
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] serial: imx: remove redundant USR2 read from FIFO reading loop

[Ilpo Järvinen]

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

On Fri, 13 Jan 2023, Sergey Organov wrote:

> There is no need to read USR2 twice at every loop iteration: get rid of the
> second read.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index b96b0edc7854..c44a7293c013 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -904,7 +904,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  
>  		rx = imx_uart_readl(sport, URXD0);
>  
> -		usr2 = imx_uart_readl(sport, USR2);
>  		if (usr2 & USR2_BRCD) {
>  			imx_uart_writel(sport, USR2_BRCD, USR2);
>  			if (uart_handle_break(&sport->port))
> 

I was already wondering why it's read more than once.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] serial: imx: stop using USR2 in FIFO reading loop

[Sergey Organov]

The chip provides all the needed bits in the URXD0 register that we read
anyway for data, so get rid of reading USR2 and use only URXD0 bits
instead.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index c44a7293c013..be00362b8b67 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -892,27 +892,21 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 	struct tty_port *port = &sport->port.state->port;
 	u32 usr2;
 
-	usr2 = imx_uart_readl(sport, USR2);
-
 	/* If we received something, check for 0xff flood */
+	usr2 = imx_uart_readl(sport, USR2);
 	if (usr2 & USR2_RDR)
 		imx_uart_check_flood(sport, usr2);
 
-	for ( ; usr2 & USR2_RDR; usr2 = imx_uart_readl(sport, USR2)) {
+	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
 		flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
-		rx = imx_uart_readl(sport, URXD0);
-
-		if (usr2 & USR2_BRCD) {
-			imx_uart_writel(sport, USR2_BRCD, USR2);
-			if (uart_handle_break(&sport->port))
-				continue;
-		}
-
 		if (unlikely(rx & URXD_ERR)) {
-			if (rx & URXD_BRK)
+			if (rx & URXD_BRK) {
 				sport->port.icount.brk++;
+				if (uart_handle_break(&sport->port))
+					continue;
+			}
 			else if (rx & URXD_PRERR)
 				sport->port.icount.parity++;
 			else if (rx & URXD_FRMERR)
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/8] serial: imx: stop using USR2 in FIFO reading loop

[Ilpo Järvinen]

On Fri, 13 Jan 2023, Sergey Organov wrote:

> The chip provides all the needed bits in the URXD0 register that we read
> anyway for data, so get rid of reading USR2 and use only URXD0 bits
> instead.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index c44a7293c013..be00362b8b67 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -892,27 +892,21 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  	struct tty_port *port = &sport->port.state->port;
>  	u32 usr2;
>  
> -	usr2 = imx_uart_readl(sport, USR2);
> -
>  	/* If we received something, check for 0xff flood */
> +	usr2 = imx_uart_readl(sport, USR2);

Please just place the read into the correct place in 2/8 rather than 
moving it needlessly here again.

-- 
 i.


>  	if (usr2 & USR2_RDR)
>  		imx_uart_check_flood(sport, usr2);
>  
> -	for ( ; usr2 & USR2_RDR; usr2 = imx_uart_readl(sport, USR2)) {
> +	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
>  		flg = TTY_NORMAL;
>  		sport->port.icount.rx++;
>  
> -		rx = imx_uart_readl(sport, URXD0);
> -
> -		if (usr2 & USR2_BRCD) {
> -			imx_uart_writel(sport, USR2_BRCD, USR2);
> -			if (uart_handle_break(&sport->port))
> -				continue;
> -		}
> -
>  		if (unlikely(rx & URXD_ERR)) {
> -			if (rx & URXD_BRK)
> +			if (rx & URXD_BRK) {
>  				sport->port.icount.brk++;
> +				if (uart_handle_break(&sport->port))
> +					continue;
> +			}
>  			else if (rx & URXD_PRERR)
>  				sport->port.icount.parity++;
>  			else if (rx & URXD_FRMERR)
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/8] serial: imx: stop using USR2 in FIFO reading loop

[Sergey Organov]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> On Fri, 13 Jan 2023, Sergey Organov wrote:
>
>> The chip provides all the needed bits in the URXD0 register that we read
>> anyway for data, so get rid of reading USR2 and use only URXD0 bits
>> instead.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 18 ++++++------------
>>  1 file changed, 6 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index c44a7293c013..be00362b8b67 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -892,27 +892,21 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>>  	struct tty_port *port = &sport->port.state->port;
>>  	u32 usr2;
>>  
>> -	usr2 = imx_uart_readl(sport, USR2);
>> -
>>  	/* If we received something, check for 0xff flood */
>> +	usr2 = imx_uart_readl(sport, USR2);
>
> Please just place the read into the correct place in 2/8 rather than 
> moving it needlessly here again.

Well, this I considered and rejected already, before publishing the
patches.

In 2/8 this read was an initialization for the entire FIFO reading loop,
the value being re-used for flood check at the beginning as well, and
with this patch it becomes just a local read for subsequent 2 lines of
code that perform flood check, not used in the FIFO loop anymore, so I
moved it in this patch to where it now belongs.

Thanks,
-- Sergey Organov

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Sergey Organov]

Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
we read registers that must not be cached.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index be00362b8b67..f4236e8995fa 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 	struct imx_port *sport = dev_id;
 	unsigned int rx, flg;
 	struct tty_port *port = &sport->port.state->port;
+	typeof(sport->port.membase) membase = sport->port.membase;
 	u32 usr2;
 
 	/* If we received something, check for 0xff flood */
-	usr2 = imx_uart_readl(sport, USR2);
+	usr2 = readl(membase + USR2);
 	if (usr2 & USR2_RDR)
 		imx_uart_check_flood(sport, usr2);
 
-	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
+	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
 		flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Ilpo Järvinen]

On Fri, 13 Jan 2023, Sergey Organov wrote:

> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
> we read registers that must not be cached.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index be00362b8b67..f4236e8995fa 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  	struct imx_port *sport = dev_id;
>  	unsigned int rx, flg;
>  	struct tty_port *port = &sport->port.state->port;
> +	typeof(sport->port.membase) membase = sport->port.membase;
>  	u32 usr2;
>  
>  	/* If we received something, check for 0xff flood */
> -	usr2 = imx_uart_readl(sport, USR2);
> +	usr2 = readl(membase + USR2);
>  	if (usr2 & USR2_RDR)
>  		imx_uart_check_flood(sport, usr2);
>  
> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
>  		flg = TTY_NORMAL;
>  		sport->port.icount.rx++;

I'd just make a uport local variable and use uport->membase + xx. There 
are plenty of sport->port constructs to replace with uport in that 
function anyway.

-- 
 i.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Sergey Organov]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> On Fri, 13 Jan 2023, Sergey Organov wrote:
>
>> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
>> we read registers that must not be cached.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index be00362b8b67..f4236e8995fa 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>>  	struct imx_port *sport = dev_id;
>>  	unsigned int rx, flg;
>>  	struct tty_port *port = &sport->port.state->port;
>> +	typeof(sport->port.membase) membase = sport->port.membase;
>>  	u32 usr2;
>>  
>>  	/* If we received something, check for 0xff flood */
>> -	usr2 = imx_uart_readl(sport, USR2);
>> +	usr2 = readl(membase + USR2);
>>  	if (usr2 & USR2_RDR)
>>  		imx_uart_check_flood(sport, usr2);
>>  
>> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
>> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
>>  		flg = TTY_NORMAL;
>>  		sport->port.icount.rx++;
>
> I'd just make a uport local variable and use uport->membase + xx. There 
> are plenty of sport->port constructs to replace with uport in that 
> function anyway.

OK, thanks, will do it this way. Probably with global rename over this
function in a separate patch?

-- 
Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Ilpo Järvinen]

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

On Tue, 17 Jan 2023, Sergey Organov wrote:

> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
> 
> > On Fri, 13 Jan 2023, Sergey Organov wrote:
> >
> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
> >> we read registers that must not be cached.
> >> 
> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> ---
> >>  drivers/tty/serial/imx.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> index be00362b8b67..f4236e8995fa 100644
> >> --- a/drivers/tty/serial/imx.c
> >> +++ b/drivers/tty/serial/imx.c
> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> >>  	struct imx_port *sport = dev_id;
> >>  	unsigned int rx, flg;
> >>  	struct tty_port *port = &sport->port.state->port;
> >> +	typeof(sport->port.membase) membase = sport->port.membase;
> >>  	u32 usr2;
> >>  
> >>  	/* If we received something, check for 0xff flood */
> >> -	usr2 = imx_uart_readl(sport, USR2);
> >> +	usr2 = readl(membase + USR2);
> >>  	if (usr2 & USR2_RDR)
> >>  		imx_uart_check_flood(sport, usr2);
> >>  
> >> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
> >> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
> >>  		flg = TTY_NORMAL;
> >>  		sport->port.icount.rx++;
> >
> > I'd just make a uport local variable and use uport->membase + xx. There 
> > are plenty of sport->port constructs to replace with uport in that 
> > function anyway.
> 
> OK, thanks, will do it this way. Probably with global rename over this
> function in a separate patch?

Yes, it is better to have it in own patch.

-- 
 i.

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Sergey Organov]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> On Tue, 17 Jan 2023, Sergey Organov wrote:
>
>> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
>> 
>> > On Fri, 13 Jan 2023, Sergey Organov wrote:
>> >
>> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
>> >> we read registers that must not be cached.
>> >> 
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >>  drivers/tty/serial/imx.c | 5 +++--
>> >>  1 file changed, 3 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> index be00362b8b67..f4236e8995fa 100644
>> >> --- a/drivers/tty/serial/imx.c
>> >> +++ b/drivers/tty/serial/imx.c
>> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>> >>  	struct imx_port *sport = dev_id;
>> >>  	unsigned int rx, flg;
>> >>  	struct tty_port *port = &sport->port.state->port;
>> >> +	typeof(sport->port.membase) membase = sport->port.membase;
>> >>  	u32 usr2;
>> >>  
>> >>  	/* If we received something, check for 0xff flood */
>> >> -	usr2 = imx_uart_readl(sport, USR2);
>> >> +	usr2 = readl(membase + USR2);
>> >>  	if (usr2 & USR2_RDR)
>> >>  		imx_uart_check_flood(sport, usr2);
>> >>  
>> >> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
>> >> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
>> >>  		flg = TTY_NORMAL;
>> >>  		sport->port.icount.rx++;
>> >
>> > I'd just make a uport local variable and use uport->membase + xx. There 
>> > are plenty of sport->port constructs to replace with uport in that 
>> > function anyway.
>> 
>> OK, thanks, will do it this way. Probably with global rename over this
>> function in a separate patch?
>
> Yes, it is better to have it in own patch.

Well, it now seems that I'll drop this patch altogether, by agreement
with Uwe. Do you think introducing of 'uport' still worth it in this
one function? I figure it's probably not, provided the reset of the code
in the driver still doesn't use the idiom.

Thanks,
-- Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Ilpo Järvinen]

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

On Wed, 18 Jan 2023, Sergey Organov wrote:

> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
> 
> > On Tue, 17 Jan 2023, Sergey Organov wrote:
> >
> >> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:
> >> 
> >> > On Fri, 13 Jan 2023, Sergey Organov wrote:
> >> >
> >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
> >> >> we read registers that must not be cached.
> >> >> 
> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> >> ---
> >> >>  drivers/tty/serial/imx.c | 5 +++--
> >> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> >> index be00362b8b67..f4236e8995fa 100644
> >> >> --- a/drivers/tty/serial/imx.c
> >> >> +++ b/drivers/tty/serial/imx.c
> >> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> >> >>  	struct imx_port *sport = dev_id;
> >> >>  	unsigned int rx, flg;
> >> >>  	struct tty_port *port = &sport->port.state->port;
> >> >> +	typeof(sport->port.membase) membase = sport->port.membase;
> >> >>  	u32 usr2;
> >> >>  
> >> >>  	/* If we received something, check for 0xff flood */
> >> >> -	usr2 = imx_uart_readl(sport, USR2);
> >> >> +	usr2 = readl(membase + USR2);
> >> >>  	if (usr2 & USR2_RDR)
> >> >>  		imx_uart_check_flood(sport, usr2);
> >> >>  
> >> >> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
> >> >> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
> >> >>  		flg = TTY_NORMAL;
> >> >>  		sport->port.icount.rx++;
> >> >
> >> > I'd just make a uport local variable and use uport->membase + xx. There 
> >> > are plenty of sport->port constructs to replace with uport in that 
> >> > function anyway.
> >> 
> >> OK, thanks, will do it this way. Probably with global rename over this
> >> function in a separate patch?
> >
> > Yes, it is better to have it in own patch.
> 
> Well, it now seems that I'll drop this patch altogether, by agreement
> with Uwe. Do you think introducing of 'uport' still worth it in this
> one function? I figure it's probably not, provided the reset of the code
> in the driver still doesn't use the idiom.

I've no strong opinion either way. So feel free to leave them as they are 
now.

-- 
 i.

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Sherry Sun]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1962 bytes --]



> -----Original Message-----
> From: Sergey Organov <sorganov@gmail.com>
> Sent: 2023Äê1ÔÂ14ÈÕ 2:44
> To: linux-serial@vger.kernel.org
> Cc: Fabio Estevam <festevam@gmail.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>; Richard
> Genoud <richard.genoud@gmail.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>;
> tharvey@gateworks.com; Tomasz Mo¨½ <tomasz.mon@camlingroup.com>;
> linux-arm-kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Sergey Organov
> <sorganov@gmail.com>
> Subject: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
> 
> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know we
> read registers that must not be cached.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> be00362b8b67..f4236e8995fa 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void
> *dev_id)
>  	struct imx_port *sport = dev_id;
>  	unsigned int rx, flg;
>  	struct tty_port *port = &sport->port.state->port;
> +	typeof(sport->port.membase) membase = sport->port.membase;
>  	u32 usr2;
> 
>  	/* If we received something, check for 0xff flood */
> -	usr2 = imx_uart_readl(sport, USR2);
> +	usr2 = readl(membase + USR2);
>  	if (usr2 & USR2_RDR)
>  		imx_uart_check_flood(sport, usr2);
> 
> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {

Actually imx_uart_readl() only set shadow registers for UCRx and UFCR, for the USR2 and URXD0 that you used here, they will not be cached.

Best Regards
Sherry


[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Sergey Organov]

Sherry Sun <sherry.sun@nxp.com> writes:

>> -----Original Message-----
>> From: Sergey Organov <sorganov@gmail.com>
>> Sent: 2023年1月14日 2:44
>> To: linux-serial@vger.kernel.org
>> Cc: Fabio Estevam <festevam@gmail.com>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>; Jiri Slaby <jirislaby@kernel.org>; Richard
>> Genoud <richard.genoud@gmail.com>; Sascha Hauer
>> <s.hauer@pengutronix.de>; Shawn Guo <shawnguo@kernel.org>;
>> tharvey@gateworks.com; Tomasz Moń <tomasz.mon@camlingroup.com>;
>> linux-arm-kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>;
>> Pengutronix Kernel Team <kernel@pengutronix.de>; Sergey Organov
>> <sorganov@gmail.com>
>> Subject: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
>> 
>> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know we
>> read registers that must not be cached.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
>> be00362b8b67..f4236e8995fa 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void
>> *dev_id)
>>  	struct imx_port *sport = dev_id;
>>  	unsigned int rx, flg;
>>  	struct tty_port *port = &sport->port.state->port;
>> +	typeof(sport->port.membase) membase = sport->port.membase;
>>  	u32 usr2;
>> 
>>  	/* If we received something, check for 0xff flood */
>> -	usr2 = imx_uart_readl(sport, USR2);
>> +	usr2 = readl(membase + USR2);
>>  	if (usr2 & USR2_RDR)
>>  		imx_uart_check_flood(sport, usr2);
>> 
>> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
>> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
>
> Actually imx_uart_readl() only set shadow registers for UCRx and UFCR,
> for the USR2 and URXD0 that you used here, they will not be cached.

Sure, and that's why we here don't need to call imx_uart_readl(), that
only needlessly checks for shadowing, thus producing pure overhead.

Best Regards,
Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1783 bytes --]

On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote:
> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
> we read registers that must not be cached.
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  drivers/tty/serial/imx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index be00362b8b67..f4236e8995fa 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  	struct imx_port *sport = dev_id;
>  	unsigned int rx, flg;
>  	struct tty_port *port = &sport->port.state->port;
> +	typeof(sport->port.membase) membase = sport->port.membase;
>  	u32 usr2;
>  
>  	/* If we received something, check for 0xff flood */
> -	usr2 = imx_uart_readl(sport, USR2);
> +	usr2 = readl(membase + USR2);
>  	if (usr2 & USR2_RDR)
>  		imx_uart_check_flood(sport, usr2);
>  
> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
>  		flg = TTY_NORMAL;
>  		sport->port.icount.rx++;

One of the motivations to introduce imx_uart_readl was to have a single
place to add a debug output to be able to inspect what the driver is
doing.

I wonder where your need for higher speed comes from and if the compiler
really generates more effective code with your change.

Please either drop the patch from your series or provide the differences
the compiler produces and a benchmark.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Sergey Organov]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote:
>> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
>> we read registers that must not be cached.
>> 
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  drivers/tty/serial/imx.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> index be00362b8b67..f4236e8995fa 100644
>> --- a/drivers/tty/serial/imx.c
>> +++ b/drivers/tty/serial/imx.c
>> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>>  	struct imx_port *sport = dev_id;
>>  	unsigned int rx, flg;
>>  	struct tty_port *port = &sport->port.state->port;
>> +	typeof(sport->port.membase) membase = sport->port.membase;
>>  	u32 usr2;
>>  
>>  	/* If we received something, check for 0xff flood */
>> -	usr2 = imx_uart_readl(sport, USR2);
>> +	usr2 = readl(membase + USR2);
>>  	if (usr2 & USR2_RDR)
>>  		imx_uart_check_flood(sport, usr2);
>>  
>> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
>> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
>>  		flg = TTY_NORMAL;
>>  		sport->port.icount.rx++;
>
> One of the motivations to introduce imx_uart_readl was to have a single
> place to add a debug output to be able to inspect what the driver is
> doing.
>
> I wonder where your need for higher speed comes from and if the compiler
> really generates more effective code with your change.

Mostly it's because I'm obviously slowing things down a bit with the
patch to fight the flood, so I feel obliged to get things back on par
with the origin. Then, higher speed, let alone the time spent with
interrupts disabled and/or spinlocks taken, is always one of generic
goals for me.

As for the generated code, with this patch I don't aim to affect code
generation, I rather avoid execution of part of existing code while
being on the most critical path. It should be quite obvious that not
executing some code is at least not slower than executing it.

>
> Please either drop the patch from your series or provide the differences
> the compiler produces and a benchmark.

If your only objection against this patch is the desire to keep a single
place to add debug output, I'll be happy to tune the resulting code to
still have one.

That said, before we make a decision, could you please tell why register
shadows that the imx_uart_readl/writel are dealing with are needed in
the first place? It looks like all the registers that are shadowed are
readable as well. What's going on here, and if it happens to be a
speed-up, do we have any benchmarks?

Thanks,
-- Sergey Organov

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 5010 bytes --]

Hello Sergey,

On Tue, Jan 17, 2023 at 04:22:51PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote:
> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
> >> we read registers that must not be cached.
> >> 
> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> ---
> >>  drivers/tty/serial/imx.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> index be00362b8b67..f4236e8995fa 100644
> >> --- a/drivers/tty/serial/imx.c
> >> +++ b/drivers/tty/serial/imx.c
> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> >>  	struct imx_port *sport = dev_id;
> >>  	unsigned int rx, flg;
> >>  	struct tty_port *port = &sport->port.state->port;
> >> +	typeof(sport->port.membase) membase = sport->port.membase;
> >>  	u32 usr2;
> >>  
> >>  	/* If we received something, check for 0xff flood */
> >> -	usr2 = imx_uart_readl(sport, USR2);
> >> +	usr2 = readl(membase + USR2);
> >>  	if (usr2 & USR2_RDR)
> >>  		imx_uart_check_flood(sport, usr2);
> >>  
> >> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
> >> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
> >>  		flg = TTY_NORMAL;
> >>  		sport->port.icount.rx++;
> >
> > One of the motivations to introduce imx_uart_readl was to have a single
> > place to add a debug output to be able to inspect what the driver is
> > doing.
> >
> > I wonder where your need for higher speed comes from and if the compiler
> > really generates more effective code with your change.
> 
> Mostly it's because I'm obviously slowing things down a bit with the
> patch to fight the flood, so I feel obliged to get things back on par
> with the origin. Then, higher speed, let alone the time spent with
> interrupts disabled and/or spinlocks taken, is always one of generic
> goals for me.
> 
> As for the generated code, with this patch I don't aim to affect code
> generation, I rather avoid execution of part of existing code while
> being on the most critical path. It should be quite obvious that not
> executing some code is at least not slower than executing it.

That's true, but I think it doesn't apply here.

I would expect that the compiler "sees" for the call

	imx_uart_readl(sport, USR2)

that the 2nd argument is constant and that for that value of offset the
call is equivalent to readl(sport->port.membase + offset);

So I doubt you're making anything quicker here.

I tried the following patch on mainline (that is without the preceding
patches in this series):

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 757825edb0cd..cfc2f7057345 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -807,7 +807,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 	unsigned int rx, flg, ignored = 0;
 	struct tty_port *port = &sport->port.state->port;
 
-	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
+	while (readl(sport->port.membase + USR2) & USR2_RDR) {
 		u32 usr2;
 
 		flg = TTY_NORMAL;

and the resulting code didn't change at all. For a bigger change (i.e.
adding a variable for sport->port.membase and replacing two
imx_uart_readl) the code changed quite a bit (it got 28 bytes bigger for
imx_v6_v7_defconfig) and in the short time I tried I couldn't judge if
the resulting code is better or not.

So a change that explicitly doesn't execute the code that the compiler
optimizes away anyhow isn't a win. Together with the fact that your
patch makes register access use different idioms and so makes it harder
to understand for a human I'd say the net benefit of your patch is negative.

> > Please either drop the patch from your series or provide the differences
> > the compiler produces and a benchmark.
> 
> If your only objection against this patch is the desire to keep a single
> place to add debug output, I'll be happy to tune the resulting code to
> still have one.

I don't see the need to optimize it.

> That said, before we make a decision, could you please tell why register
> shadows that the imx_uart_readl/writel are dealing with are needed in
> the first place? It looks like all the registers that are shadowed are
> readable as well. What's going on here, and if it happens to be a
> speed-up, do we have any benchmarks?

Not sure I did benchmarks back then, probably not. The main motivation
was really to have that single access function. So I admit being guilty
to have implemented an optimization without hard numbers just assuming
that access to (cached) RAM is quicker than the register space.

Best regards,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Sergey Organov]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> Hello Sergey,
>
> On Tue, Jan 17, 2023 at 04:22:51PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote:
>> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
>> >> we read registers that must not be cached.
>> >> 
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >>  drivers/tty/serial/imx.c | 5 +++--
>> >>  1 file changed, 3 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> index be00362b8b67..f4236e8995fa 100644
>> >> --- a/drivers/tty/serial/imx.c
>> >> +++ b/drivers/tty/serial/imx.c
>> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>> >>  	struct imx_port *sport = dev_id;
>> >>  	unsigned int rx, flg;
>> >>  	struct tty_port *port = &sport->port.state->port;
>> >> +	typeof(sport->port.membase) membase = sport->port.membase;
>> >>  	u32 usr2;
>> >>  
>> >>  	/* If we received something, check for 0xff flood */
>> >> -	usr2 = imx_uart_readl(sport, USR2);
>> >> +	usr2 = readl(membase + USR2);
>> >>  	if (usr2 & USR2_RDR)
>> >>  		imx_uart_check_flood(sport, usr2);
>> >>  
>> >> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
>> >> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
>> >>  		flg = TTY_NORMAL;
>> >>  		sport->port.icount.rx++;
>> >
>> > One of the motivations to introduce imx_uart_readl was to have a single
>> > place to add a debug output to be able to inspect what the driver is
>> > doing.
>> >
>> > I wonder where your need for higher speed comes from and if the compiler
>> > really generates more effective code with your change.
>> 
>> Mostly it's because I'm obviously slowing things down a bit with the
>> patch to fight the flood, so I feel obliged to get things back on par
>> with the origin. Then, higher speed, let alone the time spent with
>> interrupts disabled and/or spinlocks taken, is always one of generic
>> goals for me.
>> 
>> As for the generated code, with this patch I don't aim to affect code
>> generation, I rather avoid execution of part of existing code while
>> being on the most critical path. It should be quite obvious that not
>> executing some code is at least not slower than executing it.
>
> That's true, but I think it doesn't apply here.

Well, "at least not slower" still applies ;-)

>
> I would expect that the compiler "sees" for the call
>
> 	imx_uart_readl(sport, USR2)
>
> that the 2nd argument is constant and that for that value of offset the
> call is equivalent to readl(sport->port.membase + offset);
>
> So I doubt you're making anything quicker here.

Yep, it's nice compiler is clever enough to optimize-out the switch for
constant argument, though I still typically prefer to avoid over-relying
on optimizations. That said, I now tend to agree with your POV in this
particular case.

>
> I tried the following patch on mainline (that is without the preceding
> patches in this series):
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 757825edb0cd..cfc2f7057345 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -807,7 +807,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>  	unsigned int rx, flg, ignored = 0;
>  	struct tty_port *port = &sport->port.state->port;
>  
> -	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
> +	while (readl(sport->port.membase + USR2) & USR2_RDR) {
>  		u32 usr2;
>  
>  		flg = TTY_NORMAL;
>
> and the resulting code didn't change at all. For a bigger change (i.e.
> adding a variable for sport->port.membase and replacing two
> imx_uart_readl) the code changed quite a bit (it got 28 bytes bigger for
> imx_v6_v7_defconfig) and in the short time I tried I couldn't judge if
> the resulting code is better or not.
>
> So a change that explicitly doesn't execute the code that the compiler
> optimizes away anyhow isn't a win. Together with the fact that your
> patch makes register access use different idioms and so makes it harder
> to understand for a human I'd say the net benefit of your patch is
> negative.

OK, you convinced me to drop it.

>
>> > Please either drop the patch from your series or provide the differences
>> > the compiler produces and a benchmark.
>> 
>> If your only objection against this patch is the desire to keep a single
>> place to add debug output, I'll be happy to tune the resulting code to
>> still have one.
>
> I don't see the need to optimize it.
>
>> That said, before we make a decision, could you please tell why register
>> shadows that the imx_uart_readl/writel are dealing with are needed in
>> the first place? It looks like all the registers that are shadowed are
>> readable as well. What's going on here, and if it happens to be a
>> speed-up, do we have any benchmarks?
>
> Not sure I did benchmarks back then, probably not. The main motivation
> was really to have that single access function. So I admit being guilty
> to have implemented an optimization without hard numbers just assuming
> that access to (cached) RAM is quicker than the register space.

Well, even if it is quicker, we still spend time writing to both RAM and
register, and then there is no gain for the data Tx/Rx registers that
aren't cached, yet are on most critical paths.

So, if this is just caching and doesn't change behavior, I'd suggest to
get rid of the shadowing altogether, making code simpler to follow.
Besides, if it were so, I'd had no temptation to replace the
imx_uart_readl() with raw readl().

Thanks,
-- Sergey

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 7090 bytes --]

Hello Sergey,

On Wed, Jan 18, 2023 at 06:40:17PM +0300, Sergey Organov wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> > On Tue, Jan 17, 2023 at 04:22:51PM +0300, Sergey Organov wrote:
> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> >> > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote:
> >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
> >> >> we read registers that must not be cached.
> >> >> 
> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >> >> ---
> >> >>  drivers/tty/serial/imx.c | 5 +++--
> >> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> >> >> index be00362b8b67..f4236e8995fa 100644
> >> >> --- a/drivers/tty/serial/imx.c
> >> >> +++ b/drivers/tty/serial/imx.c
> >> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> >> >>  	struct imx_port *sport = dev_id;
> >> >>  	unsigned int rx, flg;
> >> >>  	struct tty_port *port = &sport->port.state->port;
> >> >> +	typeof(sport->port.membase) membase = sport->port.membase;
> >> >>  	u32 usr2;
> >> >>  
> >> >>  	/* If we received something, check for 0xff flood */
> >> >> -	usr2 = imx_uart_readl(sport, USR2);
> >> >> +	usr2 = readl(membase + USR2);
> >> >>  	if (usr2 & USR2_RDR)
> >> >>  		imx_uart_check_flood(sport, usr2);
> >> >>  
> >> >> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
> >> >> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
> >> >>  		flg = TTY_NORMAL;
> >> >>  		sport->port.icount.rx++;
> >> >
> >> > One of the motivations to introduce imx_uart_readl was to have a single
> >> > place to add a debug output to be able to inspect what the driver is
> >> > doing.
> >> >
> >> > I wonder where your need for higher speed comes from and if the compiler
> >> > really generates more effective code with your change.
> >> 
> >> Mostly it's because I'm obviously slowing things down a bit with the
> >> patch to fight the flood, so I feel obliged to get things back on par
> >> with the origin. Then, higher speed, let alone the time spent with
> >> interrupts disabled and/or spinlocks taken, is always one of generic
> >> goals for me.
> >> 
> >> As for the generated code, with this patch I don't aim to affect code
> >> generation, I rather avoid execution of part of existing code while
> >> being on the most critical path. It should be quite obvious that not
> >> executing some code is at least not slower than executing it.
> >
> > That's true, but I think it doesn't apply here.
> 
> Well, "at least not slower" still applies ;-)
> 
> >
> > I would expect that the compiler "sees" for the call
> >
> > 	imx_uart_readl(sport, USR2)
> >
> > that the 2nd argument is constant and that for that value of offset the
> > call is equivalent to readl(sport->port.membase + offset);
> >
> > So I doubt you're making anything quicker here.
> 
> Yep, it's nice compiler is clever enough to optimize-out the switch for
> constant argument, though I still typically prefer to avoid over-relying
> on optimizations. That said, I now tend to agree with your POV in this
> particular case.
> 
> >
> > I tried the following patch on mainline (that is without the preceding
> > patches in this series):
> >
> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> > index 757825edb0cd..cfc2f7057345 100644
> > --- a/drivers/tty/serial/imx.c
> > +++ b/drivers/tty/serial/imx.c
> > @@ -807,7 +807,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> >  	unsigned int rx, flg, ignored = 0;
> >  	struct tty_port *port = &sport->port.state->port;
> >  
> > -	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
> > +	while (readl(sport->port.membase + USR2) & USR2_RDR) {
> >  		u32 usr2;
> >  
> >  		flg = TTY_NORMAL;
> >
> > and the resulting code didn't change at all. For a bigger change (i.e.
> > adding a variable for sport->port.membase and replacing two
> > imx_uart_readl) the code changed quite a bit (it got 28 bytes bigger for
> > imx_v6_v7_defconfig) and in the short time I tried I couldn't judge if
> > the resulting code is better or not.
> >
> > So a change that explicitly doesn't execute the code that the compiler
> > optimizes away anyhow isn't a win. Together with the fact that your
> > patch makes register access use different idioms and so makes it harder
> > to understand for a human I'd say the net benefit of your patch is
> > negative.
> 
> OK, you convinced me to drop it.
> 
> >
> >> > Please either drop the patch from your series or provide the differences
> >> > the compiler produces and a benchmark.
> >> 
> >> If your only objection against this patch is the desire to keep a single
> >> place to add debug output, I'll be happy to tune the resulting code to
> >> still have one.
> >
> > I don't see the need to optimize it.
> >
> >> That said, before we make a decision, could you please tell why register
> >> shadows that the imx_uart_readl/writel are dealing with are needed in
> >> the first place? It looks like all the registers that are shadowed are
> >> readable as well. What's going on here, and if it happens to be a
> >> speed-up, do we have any benchmarks?
> >
> > Not sure I did benchmarks back then, probably not. The main motivation
> > was really to have that single access function. So I admit being guilty
> > to have implemented an optimization without hard numbers just assuming
> > that access to (cached) RAM is quicker than the register space.
> 
> Well, even if it is quicker, we still spend time writing to both RAM and
> register, and then there is no gain for the data Tx/Rx registers that
> aren't cached, yet are on most critical paths.

Well, assuming we're saving some time for the ctrl registers, it's worth
keeping it even though there is no gain for RX/TX, right? There is no
overhead for RX/TX.

> So, if this is just caching and doesn't change behavior, I'd suggest to
> get rid of the shadowing altogether, making code simpler to follow.

Knowing it's subjective I don't think the shadowing is complicated.
Functions are using the driver specific readl and writel functions and
shadowing is limited to these two functions.

in sum today I wouldn't change if the code does shadow the registers or
not if there isn't at least a strong hint that the one or the other
variant is better. So if you still want to work on that you're welcome,
but I invite you to do some benchmarks first and not only assume one or
the other variant is better.

My (unproved) assumption is that for console usage there is hardly a
difference and with a workflow that needs more changing of control
settings (like half duplex rs485) shadowing is slightly better.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop

[Sergey Organov]

Hello Uwe,

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

> Hello Sergey,
>
> On Wed, Jan 18, 2023 at 06:40:17PM +0300, Sergey Organov wrote:
>> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> > On Tue, Jan 17, 2023 at 04:22:51PM +0300, Sergey Organov wrote:
>> >> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> >> > On Fri, Jan 13, 2023 at 09:43:33PM +0300, Sergey Organov wrote:
>> >> >> Use readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know
>> >> >> we read registers that must not be cached.
>> >> >> 
>> >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> >> ---
>> >> >>  drivers/tty/serial/imx.c | 5 +++--
>> >> >>  1 file changed, 3 insertions(+), 2 deletions(-)
>> >> >> 
>> >> >> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> >> >> index be00362b8b67..f4236e8995fa 100644
>> >> >> --- a/drivers/tty/serial/imx.c
>> >> >> +++ b/drivers/tty/serial/imx.c
>> >> >> @@ -890,14 +890,15 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>> >> >>  	struct imx_port *sport = dev_id;
>> >> >>  	unsigned int rx, flg;
>> >> >>  	struct tty_port *port = &sport->port.state->port;
>> >> >> +	typeof(sport->port.membase) membase = sport->port.membase;
>> >> >>  	u32 usr2;
>> >> >>  
>> >> >>  	/* If we received something, check for 0xff flood */
>> >> >> -	usr2 = imx_uart_readl(sport, USR2);
>> >> >> +	usr2 = readl(membase + USR2);
>> >> >>  	if (usr2 & USR2_RDR)
>> >> >>  		imx_uart_check_flood(sport, usr2);
>> >> >>  
>> >> >> -	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
>> >> >> +	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
>> >> >>  		flg = TTY_NORMAL;
>> >> >>  		sport->port.icount.rx++;
>> >> >
>> >> > One of the motivations to introduce imx_uart_readl was to have a single
>> >> > place to add a debug output to be able to inspect what the driver is
>> >> > doing.
>> >> >
>> >> > I wonder where your need for higher speed comes from and if the compiler
>> >> > really generates more effective code with your change.
>> >> 
>> >> Mostly it's because I'm obviously slowing things down a bit with the
>> >> patch to fight the flood, so I feel obliged to get things back on par
>> >> with the origin. Then, higher speed, let alone the time spent with
>> >> interrupts disabled and/or spinlocks taken, is always one of generic
>> >> goals for me.
>> >> 
>> >> As for the generated code, with this patch I don't aim to affect code
>> >> generation, I rather avoid execution of part of existing code while
>> >> being on the most critical path. It should be quite obvious that not
>> >> executing some code is at least not slower than executing it.
>> >
>> > That's true, but I think it doesn't apply here.
>> 
>> Well, "at least not slower" still applies ;-)
>> 
>> >
>> > I would expect that the compiler "sees" for the call
>> >
>> > 	imx_uart_readl(sport, USR2)
>> >
>> > that the 2nd argument is constant and that for that value of offset the
>> > call is equivalent to readl(sport->port.membase + offset);
>> >
>> > So I doubt you're making anything quicker here.
>> 
>> Yep, it's nice compiler is clever enough to optimize-out the switch for
>> constant argument, though I still typically prefer to avoid over-relying
>> on optimizations. That said, I now tend to agree with your POV in this
>> particular case.
>> 
>> >
>> > I tried the following patch on mainline (that is without the preceding
>> > patches in this series):
>> >
>> > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
>> > index 757825edb0cd..cfc2f7057345 100644
>> > --- a/drivers/tty/serial/imx.c
>> > +++ b/drivers/tty/serial/imx.c
>> > @@ -807,7 +807,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>> >  	unsigned int rx, flg, ignored = 0;
>> >  	struct tty_port *port = &sport->port.state->port;
>> >  
>> > -	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
>> > +	while (readl(sport->port.membase + USR2) & USR2_RDR) {
>> >  		u32 usr2;
>> >  
>> >  		flg = TTY_NORMAL;
>> >
>> > and the resulting code didn't change at all. For a bigger change (i.e.
>> > adding a variable for sport->port.membase and replacing two
>> > imx_uart_readl) the code changed quite a bit (it got 28 bytes bigger for
>> > imx_v6_v7_defconfig) and in the short time I tried I couldn't judge if
>> > the resulting code is better or not.
>> >
>> > So a change that explicitly doesn't execute the code that the compiler
>> > optimizes away anyhow isn't a win. Together with the fact that your
>> > patch makes register access use different idioms and so makes it harder
>> > to understand for a human I'd say the net benefit of your patch is
>> > negative.
>> 
>> OK, you convinced me to drop it.
>> 
>> >
>> >> > Please either drop the patch from your series or provide the differences
>> >> > the compiler produces and a benchmark.
>> >> 
>> >> If your only objection against this patch is the desire to keep a single
>> >> place to add debug output, I'll be happy to tune the resulting code to
>> >> still have one.
>> >
>> > I don't see the need to optimize it.
>> >
>> >> That said, before we make a decision, could you please tell why register
>> >> shadows that the imx_uart_readl/writel are dealing with are needed in
>> >> the first place? It looks like all the registers that are shadowed are
>> >> readable as well. What's going on here, and if it happens to be a
>> >> speed-up, do we have any benchmarks?
>> >
>> > Not sure I did benchmarks back then, probably not. The main motivation
>> > was really to have that single access function. So I admit being guilty
>> > to have implemented an optimization without hard numbers just assuming
>> > that access to (cached) RAM is quicker than the register space.
>> 
>> Well, even if it is quicker, we still spend time writing to both RAM and
>> register, and then there is no gain for the data Tx/Rx registers that
>> aren't cached, yet are on most critical paths.
>
> Well, assuming we're saving some time for the ctrl registers, it's worth
> keeping it even though there is no gain for RX/TX, right? There is no
> overhead for RX/TX.
>
>> So, if this is just caching and doesn't change behavior, I'd suggest to
>> get rid of the shadowing altogether, making code simpler to follow.
>
> Knowing it's subjective I don't think the shadowing is complicated.
> Functions are using the driver specific readl and writel functions and
> shadowing is limited to these two functions.

It's not complicated indeed, but it's still code, and the less code, --
the better.

>
> in sum today I wouldn't change if the code does shadow the registers
>or not if there isn't at least a strong hint that the one or the other
>variant is better. So if you still want to work on that you're welcome,
>but I invite you to do some benchmarks first and not only assume one or
>the other variant is better.

No code is better hands down, unless proved otherwise. I dunno if a code
that somehow sneaked into the kernel gets any specific significance from
the maintenance POV though. If so, for me it'd be the only sound
argument in favor of keeping the code intact.

>
> My (unproved) assumption is that for console usage there is hardly a
> difference and with a workflow that needs more changing of control
> settings (like half duplex rs485) shadowing is slightly better.

Well, also unproved, I tend to disagree here. I'm afraid you'd rather
have hard time finding a case where it is noticeably better, especially
as on the write path the shadowing is pure overhead, and so you'd have
hard time getting this code into the kernel in the first place, provided
somebody would care to object.

For what it's worth, I've removed the shadowing in my kernel version,
and it gave me no troubles yet. For reference, I use RS232 only with DMA
turned off.

Overall, my feeling about the issue: the shadowing code is confusing and
should not have been put there in the first place. Essentially it looks
like pure code bloat, so removing it would be an improvement. Thus I
figure I'll send a separate patch for this, and you guys decide if the
patch goes in (and the code in question goes out) or not.

Thanks,
-- Sergey Organov

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 8/8] serial: imx: refine local variables in rxint()

[Sergey Organov]

The 'rx' is chip register, similar to 'usr2', so let it be of 'u32' type as
well.

Move 'flg' to be FIFO read loop local as it's not used outside.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index f4236e8995fa..45327ff6dd14 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -888,10 +888,9 @@ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
-	unsigned int rx, flg;
 	struct tty_port *port = &sport->port.state->port;
 	typeof(sport->port.membase) membase = sport->port.membase;
-	u32 usr2;
+	u32 usr2, rx;
 
 	/* If we received something, check for 0xff flood */
 	usr2 = readl(membase + USR2);
@@ -899,7 +898,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 		imx_uart_check_flood(sport, usr2);
 
 	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
-		flg = TTY_NORMAL;
+		unsigned int flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
 		if (unlikely(rx & URXD_ERR)) {
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 0/7] serial: imx: work-around for hardware RX flood, and then isr improvements

[Sergey Organov]

These series contain a work-around for hardware RX flood in first 2
commits, and then further cleanups and optimizations of the receive IRQ
handler.

WARNING: the flood fix is extensively tested with older version of the
kernel with DMA turned off. The DMA path is tested only a bit, as DMA has
receive problems on my kernel version.

Changes in v1:

  * Missing braces fixed.

  * Withdrawn patch optimizing imx_uart_readl() -> readl(), on request of
    Uwe Kleine-König.

Sergey Organov (7):
  serial: imx: factor-out common code to imx_uart_soft_reset()
  serial: imx: work-around for hardware RX flood
  serial: imx: do not sysrq broken chars
  serial: imx: do not break from FIFO reading loop prematurely
  serial: imx: remove redundant USR2 read from FIFO reading loop
  serial: imx: stop using USR2 in FIFO reading loop
  serial: imx: refine local variables in rxint()

 drivers/tty/serial/imx.c | 225 ++++++++++++++++++++++++---------------
 1 file changed, 140 insertions(+), 85 deletions(-)

Interdiff against v0:
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 45327ff6dd14..447b9060d9df 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -889,15 +889,14 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
 	struct tty_port *port = &sport->port.state->port;
-	typeof(sport->port.membase) membase = sport->port.membase;
 	u32 usr2, rx;
 
 	/* If we received something, check for 0xff flood */
-	usr2 = readl(membase + USR2);
+	usr2 = imx_uart_readl(sport, USR2);
 	if (usr2 & USR2_RDR)
 		imx_uart_check_flood(sport, usr2);
 
-	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
+	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
 		unsigned int flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
@@ -929,8 +928,9 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 				flg = TTY_OVERRUN;
 
 			sport->port.sysrq = 0;
-		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
+		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx)) {
 			continue;
+		}
 
 		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
 			continue;
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/7] serial: imx: factor-out common code to imx_uart_soft_reset()

[Sergey Organov]

We perform soft reset in 2 places, slightly differently for no sufficient
reasons, so move more generic variant to a function, and re-use the code.

Out of 2 repeat counters, 10 and 100, select 10, as the code works at
interrupts disabled, and in practice the reset happens immediately.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 73 ++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 757825edb0cd..bf222d8568a9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -397,6 +397,39 @@ static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
        hrtimer_start(hrt, ms_to_ktime(msec), HRTIMER_MODE_REL);
 }
 
+/* called with port.lock taken and irqs off */
+static void imx_uart_soft_reset(struct imx_port *sport)
+{
+	int i = 10;
+	u32 ucr2, ubir, ubmr, uts;
+
+	/*
+	 * According to the Reference Manual description of the UART SRST bit:
+	 *
+	 * "Reset the transmit and receive state machines,
+	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
+	 * and UTS[6-3]".
+	 *
+	 * We don't need to restore the old values from USR1, USR2, URXD and
+	 * UTXD. UBRC is read only, so only save/restore the other three
+	 * registers.
+	 */
+	ubir = imx_uart_readl(sport, UBIR);
+	ubmr = imx_uart_readl(sport, UBMR);
+	uts = imx_uart_readl(sport, IMX21_UTS);
+
+	ucr2 = imx_uart_readl(sport, UCR2);
+	imx_uart_writel(sport, ucr2 & ~UCR2_SRST, UCR2);
+
+	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
+		udelay(1);
+
+	/* Restore the registers */
+	imx_uart_writel(sport, ubir, UBIR);
+	imx_uart_writel(sport, ubmr, UBMR);
+	imx_uart_writel(sport, uts, IMX21_UTS);
+}
+
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1398,7 +1431,7 @@ static void imx_uart_disable_dma(struct imx_port *sport)
 static int imx_uart_startup(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
-	int retval, i;
+	int retval;
 	unsigned long flags;
 	int dma_is_inited = 0;
 	u32 ucr1, ucr2, ucr3, ucr4, uts;
@@ -1430,15 +1463,9 @@ static int imx_uart_startup(struct uart_port *port)
 		dma_is_inited = 1;
 
 	spin_lock_irqsave(&sport->port.lock, flags);
+
 	/* Reset fifo's and state machines */
-	i = 100;
-
-	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~UCR2_SRST;
-	imx_uart_writel(sport, ucr2, UCR2);
-
-	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
-		udelay(1);
+	imx_uart_soft_reset(sport);
 
 	/*
 	 * Finally, clear and enable interrupts
@@ -1593,8 +1620,6 @@ static void imx_uart_flush_buffer(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	struct scatterlist *sgl = &sport->tx_sgl[0];
-	u32 ucr2;
-	int i = 100, ubir, ubmr, uts;
 
 	if (!sport->dma_chan_tx)
 		return;
@@ -1612,32 +1637,8 @@ static void imx_uart_flush_buffer(struct uart_port *port)
 		sport->dma_is_txing = 0;
 	}
 
-	/*
-	 * According to the Reference Manual description of the UART SRST bit:
-	 *
-	 * "Reset the transmit and receive state machines,
-	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
-	 * and UTS[6-3]".
-	 *
-	 * We don't need to restore the old values from USR1, USR2, URXD and
-	 * UTXD. UBRC is read only, so only save/restore the other three
-	 * registers.
-	 */
-	ubir = imx_uart_readl(sport, UBIR);
-	ubmr = imx_uart_readl(sport, UBMR);
-	uts = imx_uart_readl(sport, IMX21_UTS);
+	imx_uart_soft_reset(sport);
 
-	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~UCR2_SRST;
-	imx_uart_writel(sport, ucr2, UCR2);
-
-	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
-		udelay(1);
-
-	/* Restore the registers */
-	imx_uart_writel(sport, ubir, UBIR);
-	imx_uart_writel(sport, ubmr, UBMR);
-	imx_uart_writel(sport, uts, IMX21_UTS);
 }
 
 static void
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/7] serial: imx: work-around for hardware RX flood

[Sergey Organov]

Check if hardware Rx flood is in progress, and issue soft reset to UART to
stop the flood.

A way to reproduce the flood (checked on iMX6SX) is: open iMX UART at 9600
8N1, and from external source send 0xf0 char at 115200 8N1. In about 90% of
cases this starts a flood of "receiving" of 0xff characters by the iMX UART
that is terminated by any activity on RxD line, or could be stopped by
issuing soft reset to the UART (just stop/start of RX does not help). Note
that in essence what we did here is sending isolated start bit about 2.4
times shorter than it is to be if issued on the UART configured baud rate.

There was earlier attempt to fix similar issue in: 'commit
b38cb7d25711 ("serial: imx: Disable new features of autobaud detection")',
but apparently it only gets harder to reproduce the issue after that
commit.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 123 ++++++++++++++++++++++++++++++---------
 1 file changed, 95 insertions(+), 28 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index bf222d8568a9..e7fce31e460d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -210,6 +210,9 @@ struct imx_port {
 
 	struct mctrl_gpios *gpios;
 
+	/* counter to stop 0xff flood */
+	int idle_counter;
+
 	/* shadow registers */
 	unsigned int ucr1;
 	unsigned int ucr2;
@@ -428,6 +431,8 @@ static void imx_uart_soft_reset(struct imx_port *sport)
 	imx_uart_writel(sport, ubir, UBIR);
 	imx_uart_writel(sport, ubmr, UBMR);
 	imx_uart_writel(sport, uts, IMX21_UTS);
+
+	sport->idle_counter = 0;
 }
 
 /* called with port.lock taken and irqs off */
@@ -834,15 +839,66 @@ static irqreturn_t imx_uart_txint(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/* Check if hardware Rx flood is in progress, and issue soft reset to stop it.
+ * This is to be called from Rx ISRs only when some bytes were actually
+ * received.
+ *
+ * A way to reproduce the flood (checked on iMX6SX) is: open iMX UART at 9600
+ * 8N1, and from external source send 0xf0 char at 115200 8N1. In about 90% of
+ * cases this starts a flood of "receiving" of 0xff characters by the iMX6 UART
+ * that is terminated by any activity on RxD line, or could be stopped by
+ * issuing soft reset to the UART (just stop/start of RX does not help). Note
+ * that what we do here is sending isolated start bit about 2.4 times shorter
+ * than it is to be on UART configured baud rate.
+ */
+static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
+{
+	/* To detect hardware 0xff flood we monitor RxD line between RX
+	 * interrupts to isolate "receiving" of char(s) with no activity
+	 * on RxD line, that'd never happen on actual data transfers.
+	 *
+	 * We use USR2_WAKE bit to check for activity on RxD line, but we have a
+	 * race here if we clear USR2_WAKE when receiving of a char is in
+	 * progress, so we might get RX interrupt later with USR2_WAKE bit
+	 * cleared. Note though that as we don't try to clear USR2_WAKE when we
+	 * detected no activity, this race may hide actual activity only once.
+	 *
+	 * Yet another case where receive interrupt may occur without RxD
+	 * activity is expiration of aging timer, so we consider this as well.
+	 *
+	 * We use 'idle_counter' to ensure that we got at least so many RX
+	 * interrupts without any detected activity on RxD line. 2 cases
+	 * described plus 1 to be on the safe side gives us a margin of 3,
+	 * below. In practice I was not able to produce a false positive to
+	 * induce soft reset at regular data transfers even using 1 as the
+	 * margin, so 3 is actually very strong.
+	 *
+	 * We count interrupts, not chars in 'idle-counter' for simplicity.
+	 */
+
+	if (usr2 & USR2_WAKE) {
+		imx_uart_writel(sport, USR2_WAKE, USR2);
+		sport->idle_counter = 0;
+	} else if (++sport->idle_counter > 3) {
+		dev_warn(sport->port.dev, "RX flood detected: soft reset.");
+		imx_uart_soft_reset(sport); /* also clears 'sport->idle_counter' */
+	}
+}
+
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
 	unsigned int rx, flg, ignored = 0;
 	struct tty_port *port = &sport->port.state->port;
+	u32 usr2;
 
-	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
-		u32 usr2;
+	usr2 = imx_uart_readl(sport, USR2);
 
+	/* If we received something, check for 0xff flood */
+	if (usr2 & USR2_RDR)
+		imx_uart_check_flood(sport, usr2);
+
+	for ( ; usr2 & USR2_RDR; usr2 = imx_uart_readl(sport, USR2)) {
 		flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
@@ -1180,55 +1236,64 @@ static void imx_uart_dma_rx_callback(void *data)
 	status = dmaengine_tx_status(chan, sport->rx_cookie, &state);
 
 	if (status == DMA_ERROR) {
+		spin_lock(&sport->port.lock);
 		imx_uart_clear_rx_errors(sport);
+		spin_unlock(&sport->port.lock);
 		return;
 	}
 
-	if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
+	/*
+	 * The state-residue variable represents the empty space
+	 * relative to the entire buffer. Taking this in consideration
+	 * the head is always calculated base on the buffer total
+	 * length - DMA transaction residue. The UART script from the
+	 * SDMA firmware will jump to the next buffer descriptor,
+	 * once a DMA transaction if finalized (IMX53 RM - A.4.1.2.4).
+	 * Taking this in consideration the tail is always at the
+	 * beginning of the buffer descriptor that contains the head.
+	 */
 
-		/*
-		 * The state-residue variable represents the empty space
-		 * relative to the entire buffer. Taking this in consideration
-		 * the head is always calculated base on the buffer total
-		 * length - DMA transaction residue. The UART script from the
-		 * SDMA firmware will jump to the next buffer descriptor,
-		 * once a DMA transaction if finalized (IMX53 RM - A.4.1.2.4).
-		 * Taking this in consideration the tail is always at the
-		 * beginning of the buffer descriptor that contains the head.
-		 */
+	/* Calculate the head */
+	rx_ring->head = sg_dma_len(sgl) - state.residue;
 
-		/* Calculate the head */
-		rx_ring->head = sg_dma_len(sgl) - state.residue;
+	/* Calculate the tail. */
+	bd_size = sg_dma_len(sgl) / sport->rx_periods;
+	rx_ring->tail = ((rx_ring->head-1) / bd_size) * bd_size;
 
-		/* Calculate the tail. */
-		bd_size = sg_dma_len(sgl) / sport->rx_periods;
-		rx_ring->tail = ((rx_ring->head-1) / bd_size) * bd_size;
+	if (rx_ring->head <= sg_dma_len(sgl) &&
+	    rx_ring->head > rx_ring->tail) {
 
-		if (rx_ring->head <= sg_dma_len(sgl) &&
-		    rx_ring->head > rx_ring->tail) {
+		/* Move data from tail to head */
+		r_bytes = rx_ring->head - rx_ring->tail;
 
-			/* Move data from tail to head */
-			r_bytes = rx_ring->head - rx_ring->tail;
+		/* If we received something, check for 0xff flood */
+		if (r_bytes > 0) {
+			spin_lock(&sport->port.lock);
+			imx_uart_check_flood(sport, imx_uart_readl(sport, USR2));
+			spin_unlock(&sport->port.lock);
+		}
+
+		if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
 
 			/* CPU claims ownership of RX DMA buffer */
 			dma_sync_sg_for_cpu(sport->port.dev, sgl, 1,
-				DMA_FROM_DEVICE);
+					    DMA_FROM_DEVICE);
 
 			w_bytes = tty_insert_flip_string(port,
-				sport->rx_buf + rx_ring->tail, r_bytes);
+							 sport->rx_buf + rx_ring->tail, r_bytes);
 
 			/* UART retrieves ownership of RX DMA buffer */
 			dma_sync_sg_for_device(sport->port.dev, sgl, 1,
-				DMA_FROM_DEVICE);
+					       DMA_FROM_DEVICE);
 
 			if (w_bytes != r_bytes)
 				sport->port.icount.buf_overrun++;
 
 			sport->port.icount.rx += w_bytes;
-		} else	{
-			WARN_ON(rx_ring->head > sg_dma_len(sgl));
-			WARN_ON(rx_ring->head <= rx_ring->tail);
 		}
+	} else	{
+		WARN_ON(rx_ring->head > sg_dma_len(sgl));
+		WARN_ON(rx_ring->head <= rx_ring->tail);
 	}
 
 	if (w_bytes) {
@@ -1304,6 +1369,8 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport)
 		imx_uart_writel(sport, USR2_ORE, USR2);
 	}
 
+	sport->idle_counter = 0;
+
 }
 
 #define TXTL_DEFAULT 2 /* reset default */
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/7] serial: imx: do not sysrq broken chars

[Sergey Organov]

Do not call uart_handle_sysrq_char() if we got any receive error along with
the character, as we don't want random junk to be considered a sysrq.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e7fce31e460d..e709118fe85c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -911,9 +911,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 				continue;
 		}
 
-		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
-			continue;
-
 		if (unlikely(rx & URXD_ERR)) {
 			if (rx & URXD_BRK)
 				sport->port.icount.brk++;
@@ -942,6 +939,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 				flg = TTY_OVERRUN;
 
 			sport->port.sysrq = 0;
+		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx)) {
+			continue;
 		}
 
 		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/7] serial: imx: do not sysrq broken chars

[Stefan Wahren]

Hi Sergey,

Am 21.01.23 um 16:36 schrieb Sergey Organov:
> Do not call uart_handle_sysrq_char() if we got any receive error along with
> the character, as we don't want random junk to be considered a sysrq.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>

this looks like a bugfix to me. Since the relevant code is pretty old, 
i'm not sure about the fixes tag here:

Fixes: 279a9acc9b72 ("2.6.11 import") ?

> ---
>   drivers/tty/serial/imx.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index e7fce31e460d..e709118fe85c 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -911,9 +911,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>   				continue;
>   		}
>   
> -		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
> -			continue;
> -
>   		if (unlikely(rx & URXD_ERR)) {
>   			if (rx & URXD_BRK)
>   				sport->port.icount.brk++;
> @@ -942,6 +939,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
>   				flg = TTY_OVERRUN;
>   
>   			sport->port.sysrq = 0;
> +		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx)) {
> +			continue;
>   		}
>   
>   		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/7] serial: imx: do not sysrq broken chars

[Sergey Organov]

Hi Stefan,

Stefan Wahren <stefan.wahren@i2se.com> writes:

> Hi Sergey,
>
> Am 21.01.23 um 16:36 schrieb Sergey Organov:
>> Do not call uart_handle_sysrq_char() if we got any receive error
>> along with the character, as we don't want random junk to be
>> considered a sysrq.
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>
> this looks like a bugfix to me. Since the relevant code is pretty old,
> i'm not sure about the fixes tag here:
>
> Fixes: 279a9acc9b72 ("2.6.11 import") ?

Dunno. I've checked a few drivers, and it seems that they don't care
either, e.g., look at atmel_serial.c, or mpc52xx_uart.c.

Either it doesn't matter, or a few drivers need similar fix? What's
going on here, I wonder?

Thanks,
-- Sergey Organov

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/7] serial: imx: do not sysrq broken chars

[Ilpo Järvinen]

On Sun, 22 Jan 2023, Sergey Organov wrote:

> Hi Stefan,
> 
> Stefan Wahren <stefan.wahren@i2se.com> writes:
> 
> > Hi Sergey,
> >
> > Am 21.01.23 um 16:36 schrieb Sergey Organov:
> >> Do not call uart_handle_sysrq_char() if we got any receive error
> >> along with the character, as we don't want random junk to be
> >> considered a sysrq.
> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >
> > this looks like a bugfix to me. Since the relevant code is pretty old,
> > i'm not sure about the fixes tag here:
> >
> > Fixes: 279a9acc9b72 ("2.6.11 import") ?
> 
> Dunno. I've checked a few drivers, and it seems that they don't care
> either, e.g., look at atmel_serial.c, or mpc52xx_uart.c.
> 
> Either it doesn't matter, or a few drivers need similar fix? What's
> going on here, I wonder?

Usually when one finds a bug from one of the drivers, the other drivers 
indeed turn out to have the same/similar bug(s).  It's not something 
uncommon.

So just fix them all, it's very much appreciated. :-) I understand it 
might not be possible to test all such fixes on those other HWs but 
usually such bugs are simple enough to fix that it isn't be a big problem.


-- 
 i.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/7] serial: imx: do not sysrq broken chars

[Sergey Organov]

Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> writes:

> On Sun, 22 Jan 2023, Sergey Organov wrote:
>
>> Hi Stefan,
>> 
>> Stefan Wahren <stefan.wahren@i2se.com> writes:
>> 
>> > Hi Sergey,
>> >
>> > Am 21.01.23 um 16:36 schrieb Sergey Organov:
>> >> Do not call uart_handle_sysrq_char() if we got any receive error
>> >> along with the character, as we don't want random junk to be
>> >> considered a sysrq.
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >
>> > this looks like a bugfix to me. Since the relevant code is pretty old,
>> > i'm not sure about the fixes tag here:
>> >
>> > Fixes: 279a9acc9b72 ("2.6.11 import") ?
>> 
>> Dunno. I've checked a few drivers, and it seems that they don't care
>> either, e.g., look at atmel_serial.c, or mpc52xx_uart.c.
>> 
>> Either it doesn't matter, or a few drivers need similar fix? What's
>> going on here, I wonder?
>
> Usually when one finds a bug from one of the drivers, the other drivers 
> indeed turn out to have the same/similar bug(s).  It's not something 
> uncommon.

Yep, it looks like deriving from the same template, with the same issue.

>
> So just fix them all, it's very much appreciated. :-) I understand it 
> might not be possible to test all such fixes on those other HWs but 
> usually such bugs are simple enough to fix that it isn't be a big problem.

I'm not even sure this is really a bug, as nobody seems to confirm it
with authority yet.

Thanks,
-- Sergey Organov

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 4/7] serial: imx: do not break from FIFO reading loop prematurely

[Sergey Organov]

There is no reason to prematurely break out of FIFO reading loop, and it
might cause needless reenters into ISR, so keep reading until FIFO is
empty.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e709118fe85c..797c441088a9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -888,7 +888,7 @@ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
-	unsigned int rx, flg, ignored = 0;
+	unsigned int rx, flg;
 	struct tty_port *port = &sport->port.state->port;
 	u32 usr2;
 
@@ -921,11 +921,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 			if (rx & URXD_OVRRUN)
 				sport->port.icount.overrun++;
 
-			if (rx & sport->port.ignore_status_mask) {
-				if (++ignored > 100)
-					goto out;
+			if (rx & sport->port.ignore_status_mask)
 				continue;
-			}
 
 			rx &= (sport->port.read_status_mask | 0xFF);
 
@@ -944,13 +941,12 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 		}
 
 		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
-			goto out;
+			continue;
 
 		if (tty_insert_flip_char(port, rx, flg) == 0)
 			sport->port.icount.buf_overrun++;
 	}
 
-out:
 	tty_flip_buffer_push(port);
 
 	return IRQ_HANDLED;
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 5/7] serial: imx: remove redundant USR2 read from FIFO reading loop

[Sergey Organov]

There is no need to read USR2 twice at every loop iteration: get rid of the
second read.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 797c441088a9..af4349fe6970 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -904,7 +904,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 
 		rx = imx_uart_readl(sport, URXD0);
 
-		usr2 = imx_uart_readl(sport, USR2);
 		if (usr2 & USR2_BRCD) {
 			imx_uart_writel(sport, USR2_BRCD, USR2);
 			if (uart_handle_break(&sport->port))
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 6/7] serial: imx: stop using USR2 in FIFO reading loop

[Sergey Organov]

The chip provides all the needed bits in the URXD0 register that we read
anyway for data, so get rid of reading USR2 and use only URXD0 bits
instead.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index af4349fe6970..488d8119562e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -892,27 +892,21 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 	struct tty_port *port = &sport->port.state->port;
 	u32 usr2;
 
-	usr2 = imx_uart_readl(sport, USR2);
-
 	/* If we received something, check for 0xff flood */
+	usr2 = imx_uart_readl(sport, USR2);
 	if (usr2 & USR2_RDR)
 		imx_uart_check_flood(sport, usr2);
 
-	for ( ; usr2 & USR2_RDR; usr2 = imx_uart_readl(sport, USR2)) {
+	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
 		flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
-		rx = imx_uart_readl(sport, URXD0);
-
-		if (usr2 & USR2_BRCD) {
-			imx_uart_writel(sport, USR2_BRCD, USR2);
-			if (uart_handle_break(&sport->port))
-				continue;
-		}
-
 		if (unlikely(rx & URXD_ERR)) {
-			if (rx & URXD_BRK)
+			if (rx & URXD_BRK) {
 				sport->port.icount.brk++;
+				if (uart_handle_break(&sport->port))
+					continue;
+			}
 			else if (rx & URXD_PRERR)
 				sport->port.icount.parity++;
 			else if (rx & URXD_FRMERR)
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 7/7] serial: imx: refine local variables in rxint()

[Sergey Organov]

The 'rx' is chip register, similar to 'usr2', so let it be of 'u32' type as
well.

Move 'flg' to be FIFO read loop local as it's not used outside.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 488d8119562e..447b9060d9df 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -888,9 +888,8 @@ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
-	unsigned int rx, flg;
 	struct tty_port *port = &sport->port.state->port;
-	u32 usr2;
+	u32 usr2, rx;
 
 	/* If we received something, check for 0xff flood */
 	usr2 = imx_uart_readl(sport, USR2);
@@ -898,7 +897,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 		imx_uart_check_flood(sport, usr2);
 
 	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
-		flg = TTY_NORMAL;
+		unsigned int flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
 		if (unlikely(rx & URXD_ERR)) {
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RESEND] serial: imx: get rid of registers shadowing

[Sergey Organov]

Neither registers shadowing is functionally needed as all the registers are
read-write, nor the shadowing makes much sense for speed-up, as most speed
critical reads/writes (of data Rx/Tx registers) are not shadowed anyway.
Moreover, the shadowing code is obviously pure overhead on the write path.

Get rid of the shadowing code and variables due to above considerations.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---

NOTE: This patch is extracted from patch series "[PATCH 0/8] serial: imx:
work-around for hardware RX flood, and then isr improvements" due to
objection of Uwe Kleine-König <u.kleine-koenig@pengutronix.de>. I still
believe this is an improvement, so I re-submit the patch separately.

 drivers/tty/serial/imx.c | 65 ++--------------------------------------
 1 file changed, 3 insertions(+), 62 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 757825edb0cd..f4e2db21d0fe 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -210,13 +210,6 @@ struct imx_port {
 
 	struct mctrl_gpios *gpios;
 
-	/* shadow registers */
-	unsigned int ucr1;
-	unsigned int ucr2;
-	unsigned int ucr3;
-	unsigned int ucr4;
-	unsigned int ufcr;
-
 	/* DMA fields */
 	unsigned int		dma_is_enabled:1;
 	unsigned int		dma_is_rxing:1;
@@ -273,59 +266,14 @@ static const struct of_device_id imx_uart_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, imx_uart_dt_ids);
 
-static void imx_uart_writel(struct imx_port *sport, u32 val, u32 offset)
+static inline void imx_uart_writel(struct imx_port *sport, u32 val, u32 offset)
 {
-	switch (offset) {
-	case UCR1:
-		sport->ucr1 = val;
-		break;
-	case UCR2:
-		sport->ucr2 = val;
-		break;
-	case UCR3:
-		sport->ucr3 = val;
-		break;
-	case UCR4:
-		sport->ucr4 = val;
-		break;
-	case UFCR:
-		sport->ufcr = val;
-		break;
-	default:
-		break;
-	}
 	writel(val, sport->port.membase + offset);
 }
 
-static u32 imx_uart_readl(struct imx_port *sport, u32 offset)
+static inline u32 imx_uart_readl(struct imx_port *sport, u32 offset)
 {
-	switch (offset) {
-	case UCR1:
-		return sport->ucr1;
-		break;
-	case UCR2:
-		/*
-		 * UCR2_SRST is the only bit in the cached registers that might
-		 * differ from the value that was last written. As it only
-		 * automatically becomes one after being cleared, reread
-		 * conditionally.
-		 */
-		if (!(sport->ucr2 & UCR2_SRST))
-			sport->ucr2 = readl(sport->port.membase + offset);
-		return sport->ucr2;
-		break;
-	case UCR3:
-		return sport->ucr3;
-		break;
-	case UCR4:
-		return sport->ucr4;
-		break;
-	case UFCR:
-		return sport->ufcr;
-		break;
-	default:
-		return readl(sport->port.membase + offset);
-	}
+	return readl(sport->port.membase + offset);
 }
 
 static inline unsigned imx_uart_uts_reg(struct imx_port *sport)
@@ -2340,13 +2288,6 @@ static int imx_uart_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* initialize shadow register values */
-	sport->ucr1 = readl(sport->port.membase + UCR1);
-	sport->ucr2 = readl(sport->port.membase + UCR2);
-	sport->ucr3 = readl(sport->port.membase + UCR3);
-	sport->ucr4 = readl(sport->port.membase + UCR4);
-	sport->ufcr = readl(sport->port.membase + UFCR);
-
 	ret = uart_get_rs485_mode(&sport->port);
 	if (ret) {
 		clk_disable_unprepare(sport->clk_ipg);

base-commit: 40c18f363a0806d4f566e8a9a9bd2d7766a72cf5
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 RESEND 0/7] serial: imx: work-around for hardware RX flood, and then isr improvements

[Sergey Organov]

These series contain a work-around for hardware RX flood in first 2
commits, and then further cleanups and optimizations of the receive IRQ
handler.

WARNING: the flood fix is extensively tested with older version of the
kernel with DMA turned off. The DMA path is tested only a bit, as DMA has
receive problems on my kernel version.

Changes in v1:

  * Missing braces fixed.

  * Withdrawn patch optimizing imx_uart_readl() -> readl(), on request of
    Uwe Kleine-König.

Sergey Organov (7):
  serial: imx: factor-out common code to imx_uart_soft_reset()
  serial: imx: work-around for hardware RX flood
  serial: imx: do not sysrq broken chars
  serial: imx: do not break from FIFO reading loop prematurely
  serial: imx: remove redundant USR2 read from FIFO reading loop
  serial: imx: stop using USR2 in FIFO reading loop
  serial: imx: refine local variables in rxint()

 drivers/tty/serial/imx.c | 225 ++++++++++++++++++++++++---------------
 1 file changed, 140 insertions(+), 85 deletions(-)

Interdiff against v0:
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 45327ff6dd14..447b9060d9df 100644
--- b/drivers/tty/serial/imx.c
+++ a/drivers/tty/serial/imx.c
@@ -889,14 +889,15 @@
 {
 	struct imx_port *sport = dev_id;
 	struct tty_port *port = &sport->port.state->port;
+	typeof(sport->port.membase) membase = sport->port.membase;
 	u32 usr2, rx;
 
 	/* If we received something, check for 0xff flood */
-	usr2 = imx_uart_readl(sport, USR2);
+	usr2 = readl(membase + USR2);
 	if (usr2 & USR2_RDR)
 		imx_uart_check_flood(sport, usr2);
 
-	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
+	while ((rx = readl(membase + URXD0)) & URXD_CHARRDY) {
 		unsigned int flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
@@ -928,9 +929,8 @@
 				flg = TTY_OVERRUN;
 
 			sport->port.sysrq = 0;
-		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx)) {
+		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
 			continue;
-		}
 
 		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
 			continue;
+- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/7] serial: imx: factor-out common code to imx_uart_soft_reset()

[Sergey Organov]

We perform soft reset in 2 places, slightly differently for no sufficient
reasons, so move more generic variant to a function, and re-use the code.

Out of 2 repeat counters, 10 and 100, select 10, as the code works at
interrupts disabled, and in practice the reset happens immediately.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 73 ++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 757825edb0cd..bf222d8568a9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -397,6 +397,39 @@ static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
        hrtimer_start(hrt, ms_to_ktime(msec), HRTIMER_MODE_REL);
 }
 
+/* called with port.lock taken and irqs off */
+static void imx_uart_soft_reset(struct imx_port *sport)
+{
+	int i = 10;
+	u32 ucr2, ubir, ubmr, uts;
+
+	/*
+	 * According to the Reference Manual description of the UART SRST bit:
+	 *
+	 * "Reset the transmit and receive state machines,
+	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
+	 * and UTS[6-3]".
+	 *
+	 * We don't need to restore the old values from USR1, USR2, URXD and
+	 * UTXD. UBRC is read only, so only save/restore the other three
+	 * registers.
+	 */
+	ubir = imx_uart_readl(sport, UBIR);
+	ubmr = imx_uart_readl(sport, UBMR);
+	uts = imx_uart_readl(sport, IMX21_UTS);
+
+	ucr2 = imx_uart_readl(sport, UCR2);
+	imx_uart_writel(sport, ucr2 & ~UCR2_SRST, UCR2);
+
+	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
+		udelay(1);
+
+	/* Restore the registers */
+	imx_uart_writel(sport, ubir, UBIR);
+	imx_uart_writel(sport, ubmr, UBMR);
+	imx_uart_writel(sport, uts, IMX21_UTS);
+}
+
 /* called with port.lock taken and irqs off */
 static void imx_uart_start_rx(struct uart_port *port)
 {
@@ -1398,7 +1431,7 @@ static void imx_uart_disable_dma(struct imx_port *sport)
 static int imx_uart_startup(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
-	int retval, i;
+	int retval;
 	unsigned long flags;
 	int dma_is_inited = 0;
 	u32 ucr1, ucr2, ucr3, ucr4, uts;
@@ -1430,15 +1463,9 @@ static int imx_uart_startup(struct uart_port *port)
 		dma_is_inited = 1;
 
 	spin_lock_irqsave(&sport->port.lock, flags);
+
 	/* Reset fifo's and state machines */
-	i = 100;
-
-	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~UCR2_SRST;
-	imx_uart_writel(sport, ucr2, UCR2);
-
-	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
-		udelay(1);
+	imx_uart_soft_reset(sport);
 
 	/*
 	 * Finally, clear and enable interrupts
@@ -1593,8 +1620,6 @@ static void imx_uart_flush_buffer(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	struct scatterlist *sgl = &sport->tx_sgl[0];
-	u32 ucr2;
-	int i = 100, ubir, ubmr, uts;
 
 	if (!sport->dma_chan_tx)
 		return;
@@ -1612,32 +1637,8 @@ static void imx_uart_flush_buffer(struct uart_port *port)
 		sport->dma_is_txing = 0;
 	}
 
-	/*
-	 * According to the Reference Manual description of the UART SRST bit:
-	 *
-	 * "Reset the transmit and receive state machines,
-	 * all FIFOs and register USR1, USR2, UBIR, UBMR, UBRC, URXD, UTXD
-	 * and UTS[6-3]".
-	 *
-	 * We don't need to restore the old values from USR1, USR2, URXD and
-	 * UTXD. UBRC is read only, so only save/restore the other three
-	 * registers.
-	 */
-	ubir = imx_uart_readl(sport, UBIR);
-	ubmr = imx_uart_readl(sport, UBMR);
-	uts = imx_uart_readl(sport, IMX21_UTS);
+	imx_uart_soft_reset(sport);
 
-	ucr2 = imx_uart_readl(sport, UCR2);
-	ucr2 &= ~UCR2_SRST;
-	imx_uart_writel(sport, ucr2, UCR2);
-
-	while (!(imx_uart_readl(sport, UCR2) & UCR2_SRST) && (--i > 0))
-		udelay(1);
-
-	/* Restore the registers */
-	imx_uart_writel(sport, ubir, UBIR);
-	imx_uart_writel(sport, ubmr, UBMR);
-	imx_uart_writel(sport, uts, IMX21_UTS);
 }
 
 static void
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/7] serial: imx: work-around for hardware RX flood

[Sergey Organov]

Check if hardware Rx flood is in progress, and issue soft reset to UART to
stop the flood.

A way to reproduce the flood (checked on iMX6SX) is: open iMX UART at 9600
8N1, and from external source send 0xf0 char at 115200 8N1. In about 90% of
cases this starts a flood of "receiving" of 0xff characters by the iMX UART
that is terminated by any activity on RxD line, or could be stopped by
issuing soft reset to the UART (just stop/start of RX does not help). Note
that in essence what we did here is sending isolated start bit about 2.4
times shorter than it is to be if issued on the UART configured baud rate.

There was earlier attempt to fix similar issue in: 'commit
b38cb7d25711 ("serial: imx: Disable new features of autobaud detection")',
but apparently it only gets harder to reproduce the issue after that
commit.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 123 ++++++++++++++++++++++++++++++---------
 1 file changed, 95 insertions(+), 28 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index bf222d8568a9..e7fce31e460d 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -210,6 +210,9 @@ struct imx_port {
 
 	struct mctrl_gpios *gpios;
 
+	/* counter to stop 0xff flood */
+	int idle_counter;
+
 	/* shadow registers */
 	unsigned int ucr1;
 	unsigned int ucr2;
@@ -428,6 +431,8 @@ static void imx_uart_soft_reset(struct imx_port *sport)
 	imx_uart_writel(sport, ubir, UBIR);
 	imx_uart_writel(sport, ubmr, UBMR);
 	imx_uart_writel(sport, uts, IMX21_UTS);
+
+	sport->idle_counter = 0;
 }
 
 /* called with port.lock taken and irqs off */
@@ -834,15 +839,66 @@ static irqreturn_t imx_uart_txint(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/* Check if hardware Rx flood is in progress, and issue soft reset to stop it.
+ * This is to be called from Rx ISRs only when some bytes were actually
+ * received.
+ *
+ * A way to reproduce the flood (checked on iMX6SX) is: open iMX UART at 9600
+ * 8N1, and from external source send 0xf0 char at 115200 8N1. In about 90% of
+ * cases this starts a flood of "receiving" of 0xff characters by the iMX6 UART
+ * that is terminated by any activity on RxD line, or could be stopped by
+ * issuing soft reset to the UART (just stop/start of RX does not help). Note
+ * that what we do here is sending isolated start bit about 2.4 times shorter
+ * than it is to be on UART configured baud rate.
+ */
+static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
+{
+	/* To detect hardware 0xff flood we monitor RxD line between RX
+	 * interrupts to isolate "receiving" of char(s) with no activity
+	 * on RxD line, that'd never happen on actual data transfers.
+	 *
+	 * We use USR2_WAKE bit to check for activity on RxD line, but we have a
+	 * race here if we clear USR2_WAKE when receiving of a char is in
+	 * progress, so we might get RX interrupt later with USR2_WAKE bit
+	 * cleared. Note though that as we don't try to clear USR2_WAKE when we
+	 * detected no activity, this race may hide actual activity only once.
+	 *
+	 * Yet another case where receive interrupt may occur without RxD
+	 * activity is expiration of aging timer, so we consider this as well.
+	 *
+	 * We use 'idle_counter' to ensure that we got at least so many RX
+	 * interrupts without any detected activity on RxD line. 2 cases
+	 * described plus 1 to be on the safe side gives us a margin of 3,
+	 * below. In practice I was not able to produce a false positive to
+	 * induce soft reset at regular data transfers even using 1 as the
+	 * margin, so 3 is actually very strong.
+	 *
+	 * We count interrupts, not chars in 'idle-counter' for simplicity.
+	 */
+
+	if (usr2 & USR2_WAKE) {
+		imx_uart_writel(sport, USR2_WAKE, USR2);
+		sport->idle_counter = 0;
+	} else if (++sport->idle_counter > 3) {
+		dev_warn(sport->port.dev, "RX flood detected: soft reset.");
+		imx_uart_soft_reset(sport); /* also clears 'sport->idle_counter' */
+	}
+}
+
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
 	unsigned int rx, flg, ignored = 0;
 	struct tty_port *port = &sport->port.state->port;
+	u32 usr2;
 
-	while (imx_uart_readl(sport, USR2) & USR2_RDR) {
-		u32 usr2;
+	usr2 = imx_uart_readl(sport, USR2);
 
+	/* If we received something, check for 0xff flood */
+	if (usr2 & USR2_RDR)
+		imx_uart_check_flood(sport, usr2);
+
+	for ( ; usr2 & USR2_RDR; usr2 = imx_uart_readl(sport, USR2)) {
 		flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
@@ -1180,55 +1236,64 @@ static void imx_uart_dma_rx_callback(void *data)
 	status = dmaengine_tx_status(chan, sport->rx_cookie, &state);
 
 	if (status == DMA_ERROR) {
+		spin_lock(&sport->port.lock);
 		imx_uart_clear_rx_errors(sport);
+		spin_unlock(&sport->port.lock);
 		return;
 	}
 
-	if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
+	/*
+	 * The state-residue variable represents the empty space
+	 * relative to the entire buffer. Taking this in consideration
+	 * the head is always calculated base on the buffer total
+	 * length - DMA transaction residue. The UART script from the
+	 * SDMA firmware will jump to the next buffer descriptor,
+	 * once a DMA transaction if finalized (IMX53 RM - A.4.1.2.4).
+	 * Taking this in consideration the tail is always at the
+	 * beginning of the buffer descriptor that contains the head.
+	 */
 
-		/*
-		 * The state-residue variable represents the empty space
-		 * relative to the entire buffer. Taking this in consideration
-		 * the head is always calculated base on the buffer total
-		 * length - DMA transaction residue. The UART script from the
-		 * SDMA firmware will jump to the next buffer descriptor,
-		 * once a DMA transaction if finalized (IMX53 RM - A.4.1.2.4).
-		 * Taking this in consideration the tail is always at the
-		 * beginning of the buffer descriptor that contains the head.
-		 */
+	/* Calculate the head */
+	rx_ring->head = sg_dma_len(sgl) - state.residue;
 
-		/* Calculate the head */
-		rx_ring->head = sg_dma_len(sgl) - state.residue;
+	/* Calculate the tail. */
+	bd_size = sg_dma_len(sgl) / sport->rx_periods;
+	rx_ring->tail = ((rx_ring->head-1) / bd_size) * bd_size;
 
-		/* Calculate the tail. */
-		bd_size = sg_dma_len(sgl) / sport->rx_periods;
-		rx_ring->tail = ((rx_ring->head-1) / bd_size) * bd_size;
+	if (rx_ring->head <= sg_dma_len(sgl) &&
+	    rx_ring->head > rx_ring->tail) {
 
-		if (rx_ring->head <= sg_dma_len(sgl) &&
-		    rx_ring->head > rx_ring->tail) {
+		/* Move data from tail to head */
+		r_bytes = rx_ring->head - rx_ring->tail;
 
-			/* Move data from tail to head */
-			r_bytes = rx_ring->head - rx_ring->tail;
+		/* If we received something, check for 0xff flood */
+		if (r_bytes > 0) {
+			spin_lock(&sport->port.lock);
+			imx_uart_check_flood(sport, imx_uart_readl(sport, USR2));
+			spin_unlock(&sport->port.lock);
+		}
+
+		if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
 
 			/* CPU claims ownership of RX DMA buffer */
 			dma_sync_sg_for_cpu(sport->port.dev, sgl, 1,
-				DMA_FROM_DEVICE);
+					    DMA_FROM_DEVICE);
 
 			w_bytes = tty_insert_flip_string(port,
-				sport->rx_buf + rx_ring->tail, r_bytes);
+							 sport->rx_buf + rx_ring->tail, r_bytes);
 
 			/* UART retrieves ownership of RX DMA buffer */
 			dma_sync_sg_for_device(sport->port.dev, sgl, 1,
-				DMA_FROM_DEVICE);
+					       DMA_FROM_DEVICE);
 
 			if (w_bytes != r_bytes)
 				sport->port.icount.buf_overrun++;
 
 			sport->port.icount.rx += w_bytes;
-		} else	{
-			WARN_ON(rx_ring->head > sg_dma_len(sgl));
-			WARN_ON(rx_ring->head <= rx_ring->tail);
 		}
+	} else	{
+		WARN_ON(rx_ring->head > sg_dma_len(sgl));
+		WARN_ON(rx_ring->head <= rx_ring->tail);
 	}
 
 	if (w_bytes) {
@@ -1304,6 +1369,8 @@ static void imx_uart_clear_rx_errors(struct imx_port *sport)
 		imx_uart_writel(sport, USR2_ORE, USR2);
 	}
 
+	sport->idle_counter = 0;
+
 }
 
 #define TXTL_DEFAULT 2 /* reset default */
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/7] serial: imx: do not sysrq broken chars

[Sergey Organov]

Do not call uart_handle_sysrq_char() if we got any receive error along with
the character, as we don't want random junk to be considered a sysrq.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e7fce31e460d..e709118fe85c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -911,9 +911,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 				continue;
 		}
 
-		if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx))
-			continue;
-
 		if (unlikely(rx & URXD_ERR)) {
 			if (rx & URXD_BRK)
 				sport->port.icount.brk++;
@@ -942,6 +939,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 				flg = TTY_OVERRUN;
 
 			sport->port.sysrq = 0;
+		} else if (uart_handle_sysrq_char(&sport->port, (unsigned char)rx)) {
+			continue;
 		}
 
 		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 4/7] serial: imx: do not break from FIFO reading loop prematurely

[Sergey Organov]

There is no reason to prematurely break out of FIFO reading loop, and it
might cause needless reenters into ISR, so keep reading until FIFO is
empty.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e709118fe85c..797c441088a9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -888,7 +888,7 @@ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
-	unsigned int rx, flg, ignored = 0;
+	unsigned int rx, flg;
 	struct tty_port *port = &sport->port.state->port;
 	u32 usr2;
 
@@ -921,11 +921,8 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 			if (rx & URXD_OVRRUN)
 				sport->port.icount.overrun++;
 
-			if (rx & sport->port.ignore_status_mask) {
-				if (++ignored > 100)
-					goto out;
+			if (rx & sport->port.ignore_status_mask)
 				continue;
-			}
 
 			rx &= (sport->port.read_status_mask | 0xFF);
 
@@ -944,13 +941,12 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 		}
 
 		if (sport->port.ignore_status_mask & URXD_DUMMY_READ)
-			goto out;
+			continue;
 
 		if (tty_insert_flip_char(port, rx, flg) == 0)
 			sport->port.icount.buf_overrun++;
 	}
 
-out:
 	tty_flip_buffer_push(port);
 
 	return IRQ_HANDLED;
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 5/7] serial: imx: remove redundant USR2 read from FIFO reading loop

[Sergey Organov]

There is no need to read USR2 twice at every loop iteration: get rid of the
second read.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 797c441088a9..af4349fe6970 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -904,7 +904,6 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 
 		rx = imx_uart_readl(sport, URXD0);
 
-		usr2 = imx_uart_readl(sport, USR2);
 		if (usr2 & USR2_BRCD) {
 			imx_uart_writel(sport, USR2_BRCD, USR2);
 			if (uart_handle_break(&sport->port))
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 6/7] serial: imx: stop using USR2 in FIFO reading loop

[Sergey Organov]

The chip provides all the needed bits in the URXD0 register that we read
anyway for data, so get rid of reading USR2 and use only URXD0 bits
instead.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index af4349fe6970..488d8119562e 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -892,27 +892,21 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 	struct tty_port *port = &sport->port.state->port;
 	u32 usr2;
 
-	usr2 = imx_uart_readl(sport, USR2);
-
 	/* If we received something, check for 0xff flood */
+	usr2 = imx_uart_readl(sport, USR2);
 	if (usr2 & USR2_RDR)
 		imx_uart_check_flood(sport, usr2);
 
-	for ( ; usr2 & USR2_RDR; usr2 = imx_uart_readl(sport, USR2)) {
+	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
 		flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
-		rx = imx_uart_readl(sport, URXD0);
-
-		if (usr2 & USR2_BRCD) {
-			imx_uart_writel(sport, USR2_BRCD, USR2);
-			if (uart_handle_break(&sport->port))
-				continue;
-		}
-
 		if (unlikely(rx & URXD_ERR)) {
-			if (rx & URXD_BRK)
+			if (rx & URXD_BRK) {
 				sport->port.icount.brk++;
+				if (uart_handle_break(&sport->port))
+					continue;
+			}
 			else if (rx & URXD_PRERR)
 				sport->port.icount.parity++;
 			else if (rx & URXD_FRMERR)
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 7/7] serial: imx: refine local variables in rxint()

[Sergey Organov]

The 'rx' is chip register, similar to 'usr2', so let it be of 'u32' type as
well.

Move 'flg' to be FIFO read loop local as it's not used outside.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 488d8119562e..447b9060d9df 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -888,9 +888,8 @@ static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
 static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 {
 	struct imx_port *sport = dev_id;
-	unsigned int rx, flg;
 	struct tty_port *port = &sport->port.state->port;
-	u32 usr2;
+	u32 usr2, rx;
 
 	/* If we received something, check for 0xff flood */
 	usr2 = imx_uart_readl(sport, USR2);
@@ -898,7 +897,7 @@ static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
 		imx_uart_check_flood(sport, usr2);
 
 	while ((rx = imx_uart_readl(sport, URXD0)) & URXD_CHARRDY) {
-		flg = TTY_NORMAL;
+		unsigned int flg = TTY_NORMAL;
 		sport->port.icount.rx++;
 
 		if (unlikely(rx & URXD_ERR)) {
-- 
2.30.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel