serial: imx: sudden rx flood: hardware bug?
Subsystems (2)
A message that is not in the archive (sent off-list or not yet received): [email protected]
Fabio Estevam
reviewerHi Sergey,
Quoted 6 lines · Sergey Organov
An effective method to reproduce the issue is to send isolated start bit at baud rate that is about 2.4 times higher than the one configured on the iMX UART while corresponding TTY is open on iMX. At these conditions the problem appears with about 90% probability, i.e., about 9 out of 10 "sent"
0xffchars provoke continuous "receiving" of0xffchars by the UART, at intervals corresponding to the UART baud rate,
I recall seeing this storm of receiving 0xff before.
I fixed it a long time ago with the following commit:
Looking at the current code, I see that the UCR3_ADNIMP bit is only set conditionally.
Could you try the change below?
drivers/tty/serial/imx.c +8 -3
@@ -2399,6 +2399,12 @@ static int imx_uart_probe(struct platform_device *pdev)imx_uart_writel (sport, ucr2,UCR2 ); } +if (!imx_uart_is_imx1 (sport)) { +u32 ucr3 =imx_uart_readl (sport,UCR3 ); + ucr3 |=UCR3_ADNIMP ; +imx_uart_writel (sport, ucr2,UCR3 ); + } +if (!imx_uart_is_imx1 (sport) && sport->dte_mode) {/* * The DCEDTE bit changes the direction of DSR, DCD, DTR and RI
@@ -2416,8 +2422,7 @@ static int imx_uart_probe(struct platform_device *pdev)* (confirmed on i.MX25) which makes them unusable. */ imx_uart_writel (sport, -IMX21_UCR3_RXDMUXSEL |UCR3_ADNIMP |UCR3_DSR , -UCR3 ); +IMX21_UCR3_RXDMUXSEL |UCR3_DSR ,UCR3 ); }else {u32 ucr3 =UCR3_DSR ;
@@ -2426,7 +2431,7 @@ static int imx_uart_probe(struct platform_device *pdev)imx_uart_writel (sport, ufcr & ~UFCR_DCEDTE ,UFCR );if (!imx_uart_is_imx1 (sport)) - ucr3 |=IMX21_UCR3_RXDMUXSEL |UCR3_ADNIMP ; + ucr3 |=IMX21_UCR3_RXDMUXSEL ;imx_uart_writel (sport, ucr3,UCR3 ); }
Fabio Estevam
reviewerdrivers/tty/serial/imx.c static int imx_uart_probe(struct platform_device *pdev) · line 2405
Quoted 4 lines · Fabio Estevam
drivers/tty/serial/imx.c +4
-0+if (!imx_uart_is_imx1 (sport)) { +u32 ucr3 =imx_uart_readl (sport,UCR3 ); + ucr3 |=UCR3_ADNIMP ; +imx_uart_writel (sport, ucr2,UCR3 );
Sorry, here I meant:imx_uart_writel(sport, ucr3, UCR3);
Hi Fabio,
Quoted 11 lines · Fabio Estevam
Hi Sergey,
Quoted 6 lines · Sergey Organov
An effective method to reproduce the issue is to send isolated start bit at baud rate that is about 2.4 times higher than the one configured on the iMX UART while corresponding TTY is open on iMX. At these conditions the problem appears with about 90% probability, i.e., about 9 out of 10 "sent"
0xffchars provoke continuous "receiving" of0xffchars by the UART, at intervals corresponding to the UART baud rate,
Quoted 5 lines
I recall seeing this storm of receiving
0xffbefore.I fixed it a long time ago with the following commit:
This patch looks correct to me, and it's there in my version as well.
Quoted 2 lines
Looking at the current code, I see that the
UCR3_ADNIMPbit is only set conditionally.
I don't see how it is relevant, as the bit is set on both ways of corresponding condition, i.e., correctly set anyway.
Quoted 2 lines
Could you try the change below?
This change looks wrong though, as the bit is now first set, and then is cleared afterwards, see below.
Anyway, I checked that UCR3_ADNIMP bit is set in UCR3 register when the problem appears in my case. Overall, I figure the problem is still there even if UCR3_ADNIMP is set, and is only more difficult to reproduce.
Quoted 6 lines
drivers/tty/serial/imx.c +0
-0
drivers/tty/serial/imx.c static int imx_uart_probe(struct platform_device *pdev) · line 2407
Quoted 9 lines
drivers/tty/serial/imx.c +6
-0@@ -2399,6 +2399,12 @@ static int imx_uart_probe(struct platform_device *pdev)imx_uart_writel (sport, ucr2,UCR2 ); } +if (!imx_uart_is_imx1 (sport)) { +u32 ucr3 =imx_uart_readl (sport,UCR3 ); + ucr3 |=UCR3_ADNIMP ; +imx_uart_writel (sport, ucr2,UCR3 ); + } +
Here we set the bit in UCR3.
drivers/tty/serial/imx.c static int imx_uart_probe(struct platform_device *pdev) · line 2425
Quoted 10 lines
drivers/tty/serial/imx.c +1
-2if (!imx_uart_is_imx1 (sport) && sport->dte_mode) {/* * The DCEDTE bit changes the direction of DSR, DCD, DTR and RI @@ -2416,8 +2422,7 @@ static int imx_uart_probe(struct platform_device *pdev)* (confirmed on i.MX25) which makes them unusable. */ imx_uart_writel (sport, -IMX21_UCR3_RXDMUXSEL |UCR3_ADNIMP |UCR3_DSR , -UCR3 ); +IMX21_UCR3_RXDMUXSEL |UCR3_DSR ,UCR3 );
And here it's now cleared.
drivers/tty/serial/imx.c static int imx_uart_probe(struct platform_device *pdev) · line 2435
Quoted 10 lines
drivers/tty/serial/imx.c +1
-1}else {u32 ucr3 =UCR3_DSR ;@@ -2426,7 +2431,7 @@ static int imx_uart_probe(struct platform_device *pdev)imx_uart_writel (sport, ufcr & ~UFCR_DCEDTE ,UFCR );if (!imx_uart_is_imx1 (sport)) - ucr3 |=IMX21_UCR3_RXDMUXSEL |UCR3_ADNIMP ; + ucr3 |=IMX21_UCR3_RXDMUXSEL ;imx_uart_writel (sport, ucr3,UCR3 );
And here it's cleared as well, as we don't read-modify-write UCR3 here.
Thanks,
-- Sergey Organov
cover serial: imx: work-around for hardware RX flood, and then isr improvements
In reply to a message not in the archiveThese 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(-)
Signature · 1 line
2.30.1
PATCH 1/8 serial: imx: factor-out common code to imx_uart_soft_reset()
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.
--- drivers/tty/serial/imx.c | 73 ++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 36 deletions(-)
drivers/tty/serial/imx.c +37 -36
@@ -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
Signature · 1 line
2.30.1
Re: [PATCH 1/8] serial: imx: factor-out common code to imx_uart_soft_reset()
Quoted 35 lines · 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 <[email protected]>--- drivers/tty/serial/imx.c | 73 ++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 36 deletions(-)drivers/tty/serial/imx.c +16
-0@@ -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.
drivers/tty/serial/imx.c static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec) · line 425
Quoted 10 lines
drivers/tty/serial/imx.c +10
-0+*/ + 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().
Signature · 1 line
i.
Quoted 80 lines
drivers/tty/serial/imx.c +10
-36+/* 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
Re: [PATCH 1/8] serial: imx: factor-out common code to imx_uart_soft_reset()
Quoted 41 lines · Ilpo Järvinen
Quoted 39 lines · 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 <[email protected]>--- drivers/tty/serial/imx.c | 73 ++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 36 deletions(-)drivers/tty/serial/imx.c +20
-0@@ -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 );
Quoted 8 lines
drivers/tty/serial/imx.c static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec) · line 425
Quoted 6 lines
drivers/tty/serial/imx.c +6
-0+ + 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
PATCH 2/8 serial: imx: work-around for hardware RX flood
In reply to Sergey OrganovCheck 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.
--- drivers/tty/serial/imx.c | 123 ++++++++++++++++++++++++++++++--------- 1 file changed, 95 insertions(+), 28 deletions(-)
drivers/tty/serial/imx.c +95 -28
@@ -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 */
Signature · 1 line
2.30.1
PATCH 3/8 serial: imx: do not sysrq broken chars
In reply to Sergey OrganovDo 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.
--- drivers/tty/serial/imx.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/tty/serial/imx.c +2 -4
@@ -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;
Signature · 1 line
2.30.1
Re: [PATCH 3/8] serial: imx: do not sysrq broken chars
Quoted 23 lines · 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 <[email protected]>--- drivers/tty/serial/imx.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)drivers/tty/serial/imx.c +0
-3@@ -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++;
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 946
Quoted 9 lines
drivers/tty/serial/imx.c +2
-1@@ -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;
Signature · 1 line
i.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 3/8] serial: imx: do not sysrq broken chars
In reply to Sergey OrganovQuoted 23 lines · 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 <[email protected]>--- drivers/tty/serial/imx.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)drivers/tty/serial/imx.c +0
-3@@ -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++;
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 943
Quoted 6 lines
drivers/tty/serial/imx.c +2
-1@@ -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.
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 946
Quoted 3 lines
drivers/tty/serial/imx.c +0
-0if (sport->port.ignore_status_mask &URXD_DUMMY_READ ) goto out;
Johan
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 3/8] serial: imx: do not sysrq broken chars
Quoted 33 lines · Johan Hovold
Quoted 23 lines · 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 <[email protected]>--- drivers/tty/serial/imx.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)drivers/tty/serial/imx.c +0
-3@@ -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++;drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 943
Quoted 6 lines
drivers/tty/serial/imx.c +2
-1@@ -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 {}
Quoted 2 lines
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.
Signature · 6 lines
Sergey _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 3/8] serial: imx: do not sysrq broken chars
Quoted 23 lines · Sergey Organov
Quoted 22 lines · Johan Hovold
Quoted 11 lines · Sergey Organov
drivers/tty/serial/imx.c +0
-3@@ -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++;drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 943
Quoted 6 lines
drivers/tty/serial/imx.c +2
-1@@ -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 {}
Quoted 5 lines
Quoted 2 lines
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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 4/8 serial: imx: do not break from FIFO reading loop prematurely
In reply to Sergey OrganovThere 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.
--- drivers/tty/serial/imx.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/tty/serial/imx.c +3 -7
@@ -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 ;
Signature · 1 line
2.30.1
PATCH 5/8 serial: imx: remove redundant USR2 read from FIFO reading loop
In reply to Sergey OrganovThere is no need to read USR2 twice at every loop iteration: get rid of the second read.
--- drivers/tty/serial/imx.c | 1 - 1 file changed, 1 deletion(-)
drivers/tty/serial/imx.c +0 -1
@@ -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))
Signature · 1 line
2.30.1
Re: [PATCH 5/8] serial: imx: remove redundant USR2 read from FIFO reading loop
Quoted 14 lines · 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 <[email protected]>--- drivers/tty/serial/imx.c | 1 - 1 file changed, 1 deletion(-)drivers/tty/serial/imx.c +0
-0
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 909
Quoted 7 lines
drivers/tty/serial/imx.c +0
-1@@ -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.
Signature · 1 line
i.
PATCH 6/8 serial: imx: stop using USR2 in FIFO reading loop
In reply to Sergey OrganovThe 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.
--- drivers/tty/serial/imx.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
drivers/tty/serial/imx.c +6 -12
@@ -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 )
Signature · 1 line
2.30.1
Re: [PATCH 6/8] serial: imx: stop using USR2 in FIFO reading loop
Quoted 14 lines · 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 <[email protected]>--- drivers/tty/serial/imx.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)drivers/tty/serial/imx.c +0
-0
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 896
Quoted 7 lines
drivers/tty/serial/imx.c +1
-2@@ -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.
Signature · 1 line
i.
Quoted 27 lines
drivers/tty/serial/imx.c +5
-10if (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 )
Re: [PATCH 6/8] serial: imx: stop using USR2 in FIFO reading loop
Quoted 19 lines · Ilpo Järvinen
Quoted 17 lines · 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 <[email protected]>--- drivers/tty/serial/imx.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)drivers/tty/serial/imx.c +0
-0@@ -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;
Quoted 7 lines
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 896
Quoted 4 lines
drivers/tty/serial/imx.c +1
-2- 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
PATCH 7/8 serial: imx: use readl() to optimize FIFO reading loop
In reply to Sergey OrganovUse readl() instead of heavier imx_uart_readl() in the Rx ISR, as we know we read registers that must not be cached.
--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/tty/serial/imx.c +3 -2
@@ -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++;
Signature · 1 line
2.30.1
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Quoted 19 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +1
-0@@ -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;
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2/* 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.
Signature · 1 line
i.
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Quoted 27 lines · Ilpo Järvinen
Quoted 25 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +2
-1@@ -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);
Quoted 8 lines
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 4 lines
drivers/tty/serial/imx.c +1
-1-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 ofsport->portconstructs 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?
Signature · 6 lines
Sergey _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Quoted 38 lines · Sergey Organov
Quoted 27 lines · Ilpo Järvinen
Quoted 25 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +2
-1@@ -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);Quoted 8 lines
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 4 lines
drivers/tty/serial/imx.c +1
-1-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 ofsport->portconstructs to replace with uport in that function anyway.
Quoted 2 lines
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.
Signature · 1 line
i.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Quoted 40 lines · Ilpo Järvinen
Quoted 38 lines · Sergey Organov
Quoted 35 lines · Ilpo Järvinen
Quoted 19 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +1
-0@@ -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;drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2/* 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 ofsport->portconstructs to replace with uport in that function anyway.
Quoted 4 lines
Quoted 2 lines
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
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Quoted 45 lines · Sergey Organov
Quoted 43 lines · Ilpo Järvinen
Quoted 40 lines · Sergey Organov
Quoted 27 lines · Ilpo Järvinen
Quoted 25 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +2
-1@@ -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);Quoted 8 lines
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 4 lines
drivers/tty/serial/imx.c +1
-1-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 ofsport->portconstructs 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?
Quoted 6 lines
Quoted 1 line
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.
Signature · 1 line
i.
RE: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
In reply to Sergey OrganovQuoted 34 lines
-----Original Message-----
From: Sergey Organov <[email protected]>
Sent: 2023年1月14日 2:44
To: [email protected]
Cc: Fabio Estevam <[email protected]>; Greg Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>; Richard Genoud <[email protected]>; Sascha Hauer <[email protected]>; Shawn Guo <[email protected]>; [email protected]; Tomasz Moń <[email protected]>; [email protected]; dl-linux-imx <[email protected]>; Pengutronix Kernel Team <[email protected]>; Sergey Organov <[email protected]>
Subject: [PATCH 7/8] serial: imx: usereadl()to optimize FIFO reading loopUse
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c index renamed from drivers/tty/serial/imx.c +0
-0
be00362b8b67..f4236e8995fa100644drivers/tty/serial/imx.c +1
-0@@ -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;
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void · line 902
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2u32 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
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Quoted 40 lines · Sherry Sun
Quoted 40 lines
-----Original Message-----
From: Sergey Organov <[email protected]>
Sent: 2023年1月14日 2:44
To: [email protected]
Cc: Fabio Estevam <[email protected]>; Greg Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>; Richard Genoud <[email protected]>; Sascha Hauer <[email protected]>; Shawn Guo <[email protected]>; [email protected]; Tomasz Moń <[email protected]>; [email protected]; dl-linux-imx <[email protected]>; Pengutronix Kernel Team <[email protected]>; Sergey Organov <[email protected]>
Subject: [PATCH 7/8] serial: imx: usereadl()to optimize FIFO reading loopUse
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c index renamed from drivers/tty/serial/imx.c +0
-0
be00362b8b67..f4236e8995fa100644drivers/tty/serial/imx.c +2
-1@@ -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);
Quoted 5 lines
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 901
Quoted 2 lines
drivers/tty/serial/imx.c +1
-1-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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
In reply to Sergey OrganovQuoted 19 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +1
-0@@ -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;
drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2/* 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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Quoted 35 lines · Uwe Kleine-König
Quoted 19 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +1
-0@@ -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;drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2/* 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_readlwas to have a single place to add a debug output to be able to inspect what the driver is doing.
Quoted 2 lines
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.
Quoted 3 lines
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
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Hello Sergey,
Quoted 45 lines · Sergey Organov
Quoted 35 lines · Uwe Kleine-König
Quoted 19 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +1
-0@@ -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;drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2/* 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_readlwas to have a single place to add a debug output to be able to inspect what the driver is doing.Quoted 2 lines
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.
Quoted 4 lines
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):
drivers/tty/serial/imx.c +1 -1
@@ -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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 5 lines
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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Quoted 48 lines · Uwe Kleine-König
Hello Sergey,
Quoted 45 lines · Sergey Organov
Quoted 35 lines · Uwe Kleine-König
Quoted 19 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +1
-0@@ -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;drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2/* 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_readlwas to have a single place to add a debug output to be able to inspect what the driver is doing.Quoted 2 lines
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.
Quoted 6 lines
Quoted 4 lines
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 ;-)
Quoted 9 lines
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.
Quoted 24 lines
I tried the following patch on mainline (that is without the preceding patches in this series):
drivers/tty/serial/imx.c +1
-1@@ -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.membaseand replacing twoimx_uart_readl) the code changed quite a bit (it got 28 bytes bigger forimx_v6_v7_defconfig) and in the short time I tried I couldn't judge if the resulting code is better or not.
Quoted 5 lines
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.
Quoted 16 lines
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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?
Quoted 4 lines
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
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Hello Sergey,
Quoted 119 lines · Sergey Organov
Quoted 46 lines · Uwe Kleine-König
Quoted 45 lines · Sergey Organov
Quoted 35 lines · Uwe Kleine-König
Quoted 19 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +1
-0@@ -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;drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2/* 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_readlwas to have a single place to add a debug output to be able to inspect what the driver is doing.Quoted 2 lines
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.
Quoted 6 lines
Quoted 4 lines
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 ;-)
Quoted 9 lines
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.
Quoted 24 lines
I tried the following patch on mainline (that is without the preceding patches in this series):
drivers/tty/serial/imx.c +1
-1@@ -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.membaseand replacing twoimx_uart_readl) the code changed quite a bit (it got 28 bytes bigger forimx_v6_v7_defconfig) and in the short time I tried I couldn't judge if the resulting code is better or not.Quoted 5 lines
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.
Quoted 16 lines
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 2 lines
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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH 7/8] serial: imx: use readl() to optimize FIFO reading loop
Hello Uwe,
Quoted 135 lines · Uwe Kleine-König
Hello Sergey,
Quoted 119 lines · Sergey Organov
Quoted 46 lines · Uwe Kleine-König
Quoted 45 lines · Sergey Organov
Quoted 35 lines · Uwe Kleine-König
Quoted 19 lines · Sergey Organov
Use
readl()instead of heavierimx_uart_readl()in the Rx ISR, as we know we read registers that must not be cached.Signed-off-by Sergey Organov <[email protected]>--- drivers/tty/serial/imx.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)drivers/tty/serial/imx.c +1
-0@@ -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;drivers/tty/serial/imx.c static irqreturn_t __imx_uart_rxint(int irq, void *dev_id) · line 903
Quoted 10 lines
drivers/tty/serial/imx.c +2
-2/* 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_readlwas to have a single place to add a debug output to be able to inspect what the driver is doing.Quoted 2 lines
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.
Quoted 6 lines
Quoted 4 lines
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 ;-)
Quoted 9 lines
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.
Quoted 24 lines
I tried the following patch on mainline (that is without the preceding patches in this series):
drivers/tty/serial/imx.c +1
-1@@ -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.membaseand replacing twoimx_uart_readl) the code changed quite a bit (it got 28 bytes bigger forimx_v6_v7_defconfig) and in the short time I tried I couldn't judge if the resulting code is better or not.Quoted 5 lines
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.
Quoted 16 lines
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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?
Quoted 8 lines
Quoted 4 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
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.
Quoted 4 lines
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
PATCH 8/8 serial: imx: refine local variables in rxint()
In reply to Sergey OrganovThe '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.
--- drivers/tty/serial/imx.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +2 -3
@@ -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 )) {
Signature · 1 line
2.30.1
cover serial: imx: work-around for hardware RX flood, and then isr improvements
In reply to a message not in the archiveThese 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:
drivers/tty/serial/imx.c +4 -4
@@ -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 ;
Signature · 1 line
2.30.1
PATCH 1/7 serial: imx: factor-out common code to imx_uart_soft_reset()
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.
--- drivers/tty/serial/imx.c | 73 ++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 36 deletions(-)
drivers/tty/serial/imx.c +37 -36
@@ -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
Signature · 1 line
2.30.1
PATCH 2/7 serial: imx: work-around for hardware RX flood
In reply to Sergey OrganovCheck 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.
--- drivers/tty/serial/imx.c | 123 ++++++++++++++++++++++++++++++--------- 1 file changed, 95 insertions(+), 28 deletions(-)
drivers/tty/serial/imx.c +95 -28
@@ -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 */
Signature · 1 line
2.30.1
PATCH 3/7 serial: imx: do not sysrq broken chars
In reply to Sergey OrganovDo 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.
--- drivers/tty/serial/imx.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +2 -3
@@ -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 )
Signature · 1 line
2.30.1
Re: [PATCH v1 3/7] serial: imx: do not sysrq broken chars
Hi Sergey,
Am 21.01.23 um 16:36 schrieb Sergey Organov:
Quoted 4 lines
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 <[email protected]>
this looks like a bugfix to me. Since the relevant code is pretty old, i'm not sure about the fixes tag here:
Quoted 27 lines
--- drivers/tty/serial/imx.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)drivers/tty/serial/imx.c +2
-3@@ -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 )
Re: [PATCH v1 3/7] serial: imx: do not sysrq broken chars
Hi Stefan,
Quoted 8 lines · Stefan Wahren
Hi Sergey,
Am 21.01.23 um 16:36 schrieb Sergey Organov:
Quoted 4 lines
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 <[email protected]>
Quoted 4 lines
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
Re: [PATCH v1 3/7] serial: imx: do not sysrq broken chars
Quoted 15 lines · Sergey Organov
Hi Stefan,
Quoted 11 lines · Stefan Wahren
Hi Sergey,
Am 21.01.23 um 16:36 schrieb Sergey Organov:
Quoted 4 lines
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 <[email protected]>this looks like a bugfix to me. Since the relevant code is pretty old, i'm not sure about the fixes tag here:
Quoted 7 lines
Quoted 1 line
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, ormpc52xx_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.
Signature · 1 line
i.
Re: [PATCH v1 3/7] serial: imx: do not sysrq broken chars
Quoted 22 lines · Ilpo Järvinen
Quoted 20 lines · Sergey Organov
Hi Stefan,
Quoted 8 lines · Stefan Wahren
Hi Sergey,
Am 21.01.23 um 16:36 schrieb Sergey Organov:
Quoted 4 lines
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 <[email protected]>Quoted 4 lines
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, ormpc52xx_uart.c.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 4 lines
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
PATCH 4/7 serial: imx: do not break from FIFO reading loop prematurely
In reply to Sergey OrganovThere 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.
--- drivers/tty/serial/imx.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/tty/serial/imx.c +3 -7
@@ -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 ;
Signature · 1 line
2.30.1
PATCH 5/7 serial: imx: remove redundant USR2 read from FIFO reading loop
In reply to Sergey OrganovThere is no need to read USR2 twice at every loop iteration: get rid of the second read.
--- drivers/tty/serial/imx.c | 1 - 1 file changed, 1 deletion(-)
drivers/tty/serial/imx.c +0 -1
@@ -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))
Signature · 1 line
2.30.1
PATCH 6/7 serial: imx: stop using USR2 in FIFO reading loop
In reply to Sergey OrganovThe 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.
--- drivers/tty/serial/imx.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
drivers/tty/serial/imx.c +6 -12
@@ -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 )
Signature · 1 line
2.30.1
PATCH 7/7 serial: imx: refine local variables in rxint()
In reply to Sergey OrganovThe '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.
--- drivers/tty/serial/imx.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +2 -3
@@ -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 )) {
Signature · 1 line
2.30.1
PATCH RESEND 1/1 serial: imx: get rid of registers shadowing
In reply to a message not in the archiveNeither 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.
---
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 <[email protected]>. 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(-)
drivers/tty/serial/imx.c +3 -62
@@ -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
Signature · 1 line
2.30.1
cover serial: imx: work-around for hardware RX flood, and then isr improvements
In reply to a message not in the archiveThese 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:
drivers/tty/serial/imx.c +4 -4
@@ -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
PATCH 1/7 serial: imx: factor-out common code to imx_uart_soft_reset()
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.
--- drivers/tty/serial/imx.c | 73 ++++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 36 deletions(-)
drivers/tty/serial/imx.c +37 -36
@@ -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
Signature · 1 line
2.30.1
PATCH 2/7 serial: imx: work-around for hardware RX flood
In reply to Sergey OrganovCheck 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.
--- drivers/tty/serial/imx.c | 123 ++++++++++++++++++++++++++++++--------- 1 file changed, 95 insertions(+), 28 deletions(-)
drivers/tty/serial/imx.c +95 -28
@@ -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 */
Signature · 1 line
2.30.1
PATCH 3/7 serial: imx: do not sysrq broken chars
In reply to Sergey OrganovDo 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.
--- drivers/tty/serial/imx.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +2 -3
@@ -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 )
Signature · 1 line
2.30.1
PATCH 4/7 serial: imx: do not break from FIFO reading loop prematurely
In reply to Sergey OrganovThere 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.
--- drivers/tty/serial/imx.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/tty/serial/imx.c +3 -7
@@ -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 ;
Signature · 1 line
2.30.1
PATCH 5/7 serial: imx: remove redundant USR2 read from FIFO reading loop
In reply to Sergey OrganovThere is no need to read USR2 twice at every loop iteration: get rid of the second read.
--- drivers/tty/serial/imx.c | 1 - 1 file changed, 1 deletion(-)
drivers/tty/serial/imx.c +0 -1
@@ -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))
Signature · 1 line
2.30.1
PATCH 6/7 serial: imx: stop using USR2 in FIFO reading loop
In reply to Sergey OrganovThe 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.
--- drivers/tty/serial/imx.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)
drivers/tty/serial/imx.c +6 -12
@@ -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 )
Signature · 1 line
2.30.1
PATCH 7/7 serial: imx: refine local variables in rxint()
In reply to Sergey OrganovThe '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.
--- drivers/tty/serial/imx.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/tty/serial/imx.c +2 -3
@@ -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 )) {
Signature · 1 line
2.30.1