* [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 @ 2022-01-19 14:52 Harald Seiler 2022-01-19 15:11 ` Uwe Kleine-König 0 siblings, 1 reply; 8+ messages in thread From: Harald Seiler @ 2022-01-19 14:52 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Harald Seiler, Jiri Slaby, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Uwe Kleine-König, Ahmad Fatoum, linux-serial, linux-arm-kernel, linux-kernel Right now, even when `delay_rts_before_send` and `delay_rts_after_send` are 0, the hrtimer is triggered (with timeout 0) which can introduce a few 100us of additional overhead on slower i.MX platforms. Implement a fast path when the delays are 0, where the RTS signal is toggled immediately instead of going through an hrtimer. This fast path behaves identical to the code before delay support was implemented. Signed-off-by: Harald Seiler <hws@denx.de> --- drivers/tty/serial/imx.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index df8a0c8b8b29..67bbbb69229d 100644 --- a/drivers/tty/serial/imx.c +++ b/drivers/tty/serial/imx.c @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port) if (port->rs485.flags & SER_RS485_ENABLED) { if (sport->tx_state == SEND) { sport->tx_state = WAIT_AFTER_SEND; - start_hrtimer_ms(&sport->trigger_stop_tx, + + if (port->rs485.delay_rts_after_send > 0) { + start_hrtimer_ms(&sport->trigger_stop_tx, port->rs485.delay_rts_after_send); - return; + return; + } + + /* continue without any delay */ } if (sport->tx_state == WAIT_AFTER_RTS || @@ -698,9 +703,14 @@ static void imx_uart_start_tx(struct uart_port *port) imx_uart_stop_rx(port); sport->tx_state = WAIT_AFTER_RTS; - start_hrtimer_ms(&sport->trigger_start_tx, + + if (port->rs485.delay_rts_before_send > 0) { + start_hrtimer_ms(&sport->trigger_start_tx, port->rs485.delay_rts_before_send); - return; + return; + } + + /* continue without any delay */ } if (sport->tx_state == WAIT_AFTER_SEND -- 2.34.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 2022-01-19 14:52 [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 Harald Seiler @ 2022-01-19 15:11 ` Uwe Kleine-König 2022-01-19 15:20 ` Harald Seiler 0 siblings, 1 reply; 8+ messages in thread From: Uwe Kleine-König @ 2022-01-19 15:11 UTC (permalink / raw) To: Harald Seiler Cc: Greg Kroah-Hartman, Fabio Estevam, Ahmad Fatoum, linux-serial, Jiri Slaby, Sascha Hauer, linux-kernel, NXP Linux Team, Pengutronix Kernel Team, Shawn Guo, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 2166 bytes --] On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote: > Right now, even when `delay_rts_before_send` and `delay_rts_after_send` > are 0, the hrtimer is triggered (with timeout 0) which can introduce a > few 100us of additional overhead on slower i.MX platforms. > > Implement a fast path when the delays are 0, where the RTS signal is > toggled immediately instead of going through an hrtimer. This fast path > behaves identical to the code before delay support was implemented. > > Signed-off-by: Harald Seiler <hws@denx.de> > --- > drivers/tty/serial/imx.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index df8a0c8b8b29..67bbbb69229d 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port) > if (port->rs485.flags & SER_RS485_ENABLED) { > if (sport->tx_state == SEND) { > sport->tx_state = WAIT_AFTER_SEND; > - start_hrtimer_ms(&sport->trigger_stop_tx, > + > + if (port->rs485.delay_rts_after_send > 0) { > + start_hrtimer_ms(&sport->trigger_stop_tx, > port->rs485.delay_rts_after_send); > - return; > + return; > + } > + > + /* continue without any delay */ Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ? > } > > if (sport->tx_state == WAIT_AFTER_RTS || > @@ -698,9 +703,14 @@ static void imx_uart_start_tx(struct uart_port *port) > imx_uart_stop_rx(port); > > sport->tx_state = WAIT_AFTER_RTS; > - start_hrtimer_ms(&sport->trigger_start_tx, > + > + if (port->rs485.delay_rts_before_send > 0) { > + start_hrtimer_ms(&sport->trigger_start_tx, > port->rs485.delay_rts_before_send); > - return; > + return; > + } > + > + /* continue without any delay */ Here similar question here about sport->tx_state = WAIT_AFTER_RTS; Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 2022-01-19 15:11 ` Uwe Kleine-König @ 2022-01-19 15:20 ` Harald Seiler 2022-01-19 16:21 ` Uwe Kleine-König 0 siblings, 1 reply; 8+ messages in thread From: Harald Seiler @ 2022-01-19 15:20 UTC (permalink / raw) To: Uwe Kleine-König Cc: Greg Kroah-Hartman, Fabio Estevam, Ahmad Fatoum, linux-serial, Jiri Slaby, Sascha Hauer, linux-kernel, NXP Linux Team, Pengutronix Kernel Team, Shawn Guo, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 2790 bytes --] Hi, On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote: > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote: > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send` > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a > > few 100us of additional overhead on slower i.MX platforms. > > > > Implement a fast path when the delays are 0, where the RTS signal is > > toggled immediately instead of going through an hrtimer. This fast path > > behaves identical to the code before delay support was implemented. > > > > Signed-off-by: Harald Seiler <hws@denx.de> > > --- > > drivers/tty/serial/imx.c | 18 ++++++++++++++---- > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > > index df8a0c8b8b29..67bbbb69229d 100644 > > --- a/drivers/tty/serial/imx.c > > +++ b/drivers/tty/serial/imx.c > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port) > > if (port->rs485.flags & SER_RS485_ENABLED) { > > if (sport->tx_state == SEND) { > > sport->tx_state = WAIT_AFTER_SEND; > > - start_hrtimer_ms(&sport->trigger_stop_tx, > > + > > + if (port->rs485.delay_rts_after_send > 0) { > > + start_hrtimer_ms(&sport->trigger_stop_tx, > > port->rs485.delay_rts_after_send); > > - return; > > + return; > > + } > > + > > + /* continue without any delay */ > > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ? I am keeping the assignment intentionally, to fall into the if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF). I originally had the code structured like this: if (port->rs485.delay_rts_after_send > 0) { sport->tx_state = WAIT_AFTER_SEND; start_hrtimer_ms(&sport->trigger_stop_tx, port->rs485.delay_rts_after_send); return; } else { /* continue without any delay */ sport->tx_state = WAIT_AFTER_SEND; } This is functionally identical, but maybe a bit more explicit. Not sure what is more clear to read? > > } > > > > if (sport->tx_state == WAIT_AFTER_RTS || > > @@ -698,9 +703,14 @@ static void imx_uart_start_tx(struct uart_port *port) > > imx_uart_stop_rx(port); > > > > sport->tx_state = WAIT_AFTER_RTS; > > - start_hrtimer_ms(&sport->trigger_start_tx, > > + > > + if (port->rs485.delay_rts_before_send > 0) { > > + start_hrtimer_ms(&sport->trigger_start_tx, > > port->rs485.delay_rts_before_send); > > - return; > > + return; > > + } > > + > > + /* continue without any delay */ > > Here similar question here about sport->tx_state = WAIT_AFTER_RTS; Same as above, but with WAIT_AFTER_RTS of course... -- Harald [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 850 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 2022-01-19 15:20 ` Harald Seiler @ 2022-01-19 16:21 ` Uwe Kleine-König 2022-01-19 16:59 ` Harald Seiler 0 siblings, 1 reply; 8+ messages in thread From: Uwe Kleine-König @ 2022-01-19 16:21 UTC (permalink / raw) To: Harald Seiler Cc: Ahmad Fatoum, linux-serial, Greg Kroah-Hartman, Sascha Hauer, linux-kernel, NXP Linux Team, Fabio Estevam, Shawn Guo, Jiri Slaby, Pengutronix Kernel Team, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 2696 bytes --] On Wed, Jan 19, 2022 at 04:20:12PM +0100, Harald Seiler wrote: > Hi, > > On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote: > > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote: > > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send` > > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a > > > few 100us of additional overhead on slower i.MX platforms. > > > > > > Implement a fast path when the delays are 0, where the RTS signal is > > > toggled immediately instead of going through an hrtimer. This fast path > > > behaves identical to the code before delay support was implemented. > > > > > > Signed-off-by: Harald Seiler <hws@denx.de> > > > --- > > > drivers/tty/serial/imx.c | 18 ++++++++++++++---- > > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > > > index df8a0c8b8b29..67bbbb69229d 100644 > > > --- a/drivers/tty/serial/imx.c > > > +++ b/drivers/tty/serial/imx.c > > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port) > > > if (port->rs485.flags & SER_RS485_ENABLED) { > > > if (sport->tx_state == SEND) { > > > sport->tx_state = WAIT_AFTER_SEND; > > > - start_hrtimer_ms(&sport->trigger_stop_tx, > > > + > > > + if (port->rs485.delay_rts_after_send > 0) { > > > + start_hrtimer_ms(&sport->trigger_stop_tx, > > > port->rs485.delay_rts_after_send); > > > - return; > > > + return; > > > + } > > > + > > > + /* continue without any delay */ > > > > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ? > > I am keeping the assignment intentionally, to fall into the > if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF). > I originally had the code structured like this: > > if (port->rs485.delay_rts_after_send > 0) { > sport->tx_state = WAIT_AFTER_SEND; > start_hrtimer_ms(&sport->trigger_stop_tx, > port->rs485.delay_rts_after_send); > return; > } else { > /* continue without any delay */ > sport->tx_state = WAIT_AFTER_SEND; > } > > This is functionally identical, but maybe a bit more explicit. > > Not sure what is more clear to read? I didn't oppose to the readability thing. With your patch you skip starting the stop_tx timer and that would usually care for calling imx_uart_stop_tx and setting sport->tx_state = OFF. This doesn't happen with your patch any more. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 2022-01-19 16:21 ` Uwe Kleine-König @ 2022-01-19 16:59 ` Harald Seiler 2022-02-08 10:03 ` Greg Kroah-Hartman 0 siblings, 1 reply; 8+ messages in thread From: Harald Seiler @ 2022-01-19 16:59 UTC (permalink / raw) To: Uwe Kleine-König Cc: Ahmad Fatoum, linux-serial, Greg Kroah-Hartman, Sascha Hauer, linux-kernel, NXP Linux Team, Fabio Estevam, Shawn Guo, Jiri Slaby, Pengutronix Kernel Team, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 3660 bytes --] Hi, On Wed, 2022-01-19 at 17:21 +0100, Uwe Kleine-König wrote: > On Wed, Jan 19, 2022 at 04:20:12PM +0100, Harald Seiler wrote: > > Hi, > > > > On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote: > > > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote: > > > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send` > > > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a > > > > few 100us of additional overhead on slower i.MX platforms. > > > > > > > > Implement a fast path when the delays are 0, where the RTS signal is > > > > toggled immediately instead of going through an hrtimer. This fast path > > > > behaves identical to the code before delay support was implemented. > > > > > > > > Signed-off-by: Harald Seiler <hws@denx.de> > > > > --- > > > > drivers/tty/serial/imx.c | 18 ++++++++++++++---- > > > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > > > > index df8a0c8b8b29..67bbbb69229d 100644 > > > > --- a/drivers/tty/serial/imx.c > > > > +++ b/drivers/tty/serial/imx.c > > > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port) > > > > if (port->rs485.flags & SER_RS485_ENABLED) { > > > > if (sport->tx_state == SEND) { > > > > sport->tx_state = WAIT_AFTER_SEND; > > > > - start_hrtimer_ms(&sport->trigger_stop_tx, > > > > + > > > > + if (port->rs485.delay_rts_after_send > 0) { > > > > + start_hrtimer_ms(&sport->trigger_stop_tx, > > > > port->rs485.delay_rts_after_send); > > > > - return; > > > > + return; > > > > + } > > > > + > > > > + /* continue without any delay */ > > > > > > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ? > > > > I am keeping the assignment intentionally, to fall into the > > if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF). > > I originally had the code structured like this: > > > > if (port->rs485.delay_rts_after_send > 0) { > > sport->tx_state = WAIT_AFTER_SEND; > > start_hrtimer_ms(&sport->trigger_stop_tx, > > port->rs485.delay_rts_after_send); > > return; > > } else { > > /* continue without any delay */ > > sport->tx_state = WAIT_AFTER_SEND; > > } > > > > This is functionally identical, but maybe a bit more explicit. > > > > Not sure what is more clear to read? > > I didn't oppose to the readability thing. With your patch you skip > starting the stop_tx timer and that would usually care for calling > imx_uart_stop_tx and setting sport->tx_state = OFF. This doesn't happen > with your patch any more. Not starting the timer is the entire point of the patch - instead, the code which would run inside the timer callback now runs immediately. To do this, I set the tx_state to WAIT_AFTER_SEND and _don't_ do the early return which leads into the if(tx_state == WAIT_AFTER_SEND) below. This is the code-path which normally runs later in the hrtimer callback. I suppose it would have been good to provide more context lines in the patch... Here is the relevant bit (in the changed version now): if (sport->tx_state == SEND) { sport->tx_state = WAIT_AFTER_SEND; if (port->rs485.delay_rts_after_send > 0) { start_hrtimer_ms(&sport->trigger_stop_tx, port->rs485.delay_rts_after_send); return; } /* continue without any delay */ } if (sport->tx_state == WAIT_AFTER_RTS || sport->tx_state == WAIT_AFTER_SEND) { /* ... actual rts toggling ... */ sport->tx_state = OFF; } Regards, -- Harald [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 850 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 2022-01-19 16:59 ` Harald Seiler @ 2022-02-08 10:03 ` Greg Kroah-Hartman 2022-02-08 11:12 ` Uwe Kleine-König 0 siblings, 1 reply; 8+ messages in thread From: Greg Kroah-Hartman @ 2022-02-08 10:03 UTC (permalink / raw) To: Harald Seiler Cc: Uwe Kleine-König, Ahmad Fatoum, linux-serial, Sascha Hauer, linux-kernel, NXP Linux Team, Fabio Estevam, Shawn Guo, Jiri Slaby, Pengutronix Kernel Team, linux-arm-kernel On Wed, Jan 19, 2022 at 05:59:46PM +0100, Harald Seiler wrote: > Hi, > > On Wed, 2022-01-19 at 17:21 +0100, Uwe Kleine-König wrote: > > On Wed, Jan 19, 2022 at 04:20:12PM +0100, Harald Seiler wrote: > > > Hi, > > > > > > On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote: > > > > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote: > > > > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send` > > > > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a > > > > > few 100us of additional overhead on slower i.MX platforms. > > > > > > > > > > Implement a fast path when the delays are 0, where the RTS signal is > > > > > toggled immediately instead of going through an hrtimer. This fast path > > > > > behaves identical to the code before delay support was implemented. > > > > > > > > > > Signed-off-by: Harald Seiler <hws@denx.de> > > > > > --- > > > > > drivers/tty/serial/imx.c | 18 ++++++++++++++---- > > > > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > > > > > index df8a0c8b8b29..67bbbb69229d 100644 > > > > > --- a/drivers/tty/serial/imx.c > > > > > +++ b/drivers/tty/serial/imx.c > > > > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port) > > > > > if (port->rs485.flags & SER_RS485_ENABLED) { > > > > > if (sport->tx_state == SEND) { > > > > > sport->tx_state = WAIT_AFTER_SEND; > > > > > - start_hrtimer_ms(&sport->trigger_stop_tx, > > > > > + > > > > > + if (port->rs485.delay_rts_after_send > 0) { > > > > > + start_hrtimer_ms(&sport->trigger_stop_tx, > > > > > port->rs485.delay_rts_after_send); > > > > > - return; > > > > > + return; > > > > > + } > > > > > + > > > > > + /* continue without any delay */ > > > > > > > > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ? > > > > > > I am keeping the assignment intentionally, to fall into the > > > if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF). > > > I originally had the code structured like this: > > > > > > if (port->rs485.delay_rts_after_send > 0) { > > > sport->tx_state = WAIT_AFTER_SEND; > > > start_hrtimer_ms(&sport->trigger_stop_tx, > > > port->rs485.delay_rts_after_send); > > > return; > > > } else { > > > /* continue without any delay */ > > > sport->tx_state = WAIT_AFTER_SEND; > > > } > > > > > > This is functionally identical, but maybe a bit more explicit. > > > > > > Not sure what is more clear to read? > > > > I didn't oppose to the readability thing. With your patch you skip > > starting the stop_tx timer and that would usually care for calling > > imx_uart_stop_tx and setting sport->tx_state = OFF. This doesn't happen > > with your patch any more. > > Not starting the timer is the entire point of the patch - instead, the > code which would run inside the timer callback now runs immediately. To > do this, I set the tx_state to WAIT_AFTER_SEND and _don't_ do the early > return which leads into the if(tx_state == WAIT_AFTER_SEND) below. This > is the code-path which normally runs later in the hrtimer callback. > > I suppose it would have been good to provide more context lines in the > patch... Here is the relevant bit (in the changed version now): > > if (sport->tx_state == SEND) { > sport->tx_state = WAIT_AFTER_SEND; > > if (port->rs485.delay_rts_after_send > 0) { > start_hrtimer_ms(&sport->trigger_stop_tx, > port->rs485.delay_rts_after_send); > return; > } > > /* continue without any delay */ > } > > if (sport->tx_state == WAIT_AFTER_RTS || > sport->tx_state == WAIT_AFTER_SEND) { > /* ... actual rts toggling ... */ > > sport->tx_state = OFF; > } > Uwe, any thoughts about if this patch should be taken or not? thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 2022-02-08 10:03 ` Greg Kroah-Hartman @ 2022-02-08 11:12 ` Uwe Kleine-König 2022-02-09 15:30 ` Uwe Kleine-König 0 siblings, 1 reply; 8+ messages in thread From: Uwe Kleine-König @ 2022-02-08 11:12 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Harald Seiler, Ahmad Fatoum, Pengutronix Kernel Team, Jiri Slaby, Shawn Guo, Sascha Hauer, linux-kernel, NXP Linux Team, linux-serial, Fabio Estevam, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 4458 bytes --] Hello Greg, On Tue, Feb 08, 2022 at 11:03:56AM +0100, Greg Kroah-Hartman wrote: > On Wed, Jan 19, 2022 at 05:59:46PM +0100, Harald Seiler wrote: > > On Wed, 2022-01-19 at 17:21 +0100, Uwe Kleine-König wrote: > > > On Wed, Jan 19, 2022 at 04:20:12PM +0100, Harald Seiler wrote: > > > > Hi, > > > > > > > > On Wed, 2022-01-19 at 16:11 +0100, Uwe Kleine-König wrote: > > > > > On Wed, Jan 19, 2022 at 03:52:03PM +0100, Harald Seiler wrote: > > > > > > Right now, even when `delay_rts_before_send` and `delay_rts_after_send` > > > > > > are 0, the hrtimer is triggered (with timeout 0) which can introduce a > > > > > > few 100us of additional overhead on slower i.MX platforms. > > > > > > > > > > > > Implement a fast path when the delays are 0, where the RTS signal is > > > > > > toggled immediately instead of going through an hrtimer. This fast path > > > > > > behaves identical to the code before delay support was implemented. > > > > > > > > > > > > Signed-off-by: Harald Seiler <hws@denx.de> > > > > > > --- > > > > > > drivers/tty/serial/imx.c | 18 ++++++++++++++---- > > > > > > 1 file changed, 14 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > > > > > > index df8a0c8b8b29..67bbbb69229d 100644 > > > > > > --- a/drivers/tty/serial/imx.c > > > > > > +++ b/drivers/tty/serial/imx.c > > > > > > @@ -455,9 +455,14 @@ static void imx_uart_stop_tx(struct uart_port *port) > > > > > > if (port->rs485.flags & SER_RS485_ENABLED) { > > > > > > if (sport->tx_state == SEND) { > > > > > > sport->tx_state = WAIT_AFTER_SEND; > > > > > > - start_hrtimer_ms(&sport->trigger_stop_tx, > > > > > > + > > > > > > + if (port->rs485.delay_rts_after_send > 0) { > > > > > > + start_hrtimer_ms(&sport->trigger_stop_tx, > > > > > > port->rs485.delay_rts_after_send); > > > > > > - return; > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > + /* continue without any delay */ > > > > > > > > > > Is it right to keep the assignment sport->tx_state = WAIT_AFTER_SEND ? > > > > > > > > I am keeping the assignment intentionally, to fall into the > > > > if(state == WAIT_AFTER_RTS) below (which then sets the state to OFF). > > > > I originally had the code structured like this: > > > > > > > > if (port->rs485.delay_rts_after_send > 0) { > > > > sport->tx_state = WAIT_AFTER_SEND; > > > > start_hrtimer_ms(&sport->trigger_stop_tx, > > > > port->rs485.delay_rts_after_send); > > > > return; > > > > } else { > > > > /* continue without any delay */ > > > > sport->tx_state = WAIT_AFTER_SEND; > > > > } > > > > > > > > This is functionally identical, but maybe a bit more explicit. > > > > > > > > Not sure what is more clear to read? > > > > > > I didn't oppose to the readability thing. With your patch you skip > > > starting the stop_tx timer and that would usually care for calling > > > imx_uart_stop_tx and setting sport->tx_state = OFF. This doesn't happen > > > with your patch any more. > > > > Not starting the timer is the entire point of the patch - instead, the > > code which would run inside the timer callback now runs immediately. To > > do this, I set the tx_state to WAIT_AFTER_SEND and _don't_ do the early > > return which leads into the if(tx_state == WAIT_AFTER_SEND) below. This > > is the code-path which normally runs later in the hrtimer callback. > > > > I suppose it would have been good to provide more context lines in the > > patch... Here is the relevant bit (in the changed version now): > > > > if (sport->tx_state == SEND) { > > sport->tx_state = WAIT_AFTER_SEND; > > > > if (port->rs485.delay_rts_after_send > 0) { > > start_hrtimer_ms(&sport->trigger_stop_tx, > > port->rs485.delay_rts_after_send); > > return; > > } > > > > /* continue without any delay */ > > } > > > > if (sport->tx_state == WAIT_AFTER_RTS || > > sport->tx_state == WAIT_AFTER_SEND) { > > /* ... actual rts toggling ... */ > > > > sport->tx_state = OFF; > > } > > > > Uwe, any thoughts about if this patch should be taken or not? I will take a deeper look later today and tell you my thoughts. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 2022-02-08 11:12 ` Uwe Kleine-König @ 2022-02-09 15:30 ` Uwe Kleine-König 0 siblings, 0 replies; 8+ messages in thread From: Uwe Kleine-König @ 2022-02-09 15:30 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Fabio Estevam, Ahmad Fatoum, linux-serial, Shawn Guo, Sascha Hauer, linux-kernel, NXP Linux Team, Pengutronix Kernel Team, Harald Seiler, Jiri Slaby, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 643 bytes --] Hello Greg, On Tue, Feb 08, 2022 at 12:12:03PM +0100, Uwe Kleine-König wrote: > On Tue, Feb 08, 2022 at 11:03:56AM +0100, Greg Kroah-Hartman wrote: > > Uwe, any thoughts about if this patch should be taken or not? > > I will take a deeper look later today and tell you my thoughts. It took a bit longer, but after looking again with the driver open in my editor I agree that patch is fine. Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-09 15:30 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-19 14:52 [PATCH] tty: serial: imx: Add fast path when rs485 delays are 0 Harald Seiler 2022-01-19 15:11 ` Uwe Kleine-König 2022-01-19 15:20 ` Harald Seiler 2022-01-19 16:21 ` Uwe Kleine-König 2022-01-19 16:59 ` Harald Seiler 2022-02-08 10:03 ` Greg Kroah-Hartman 2022-02-08 11:12 ` Uwe Kleine-König 2022-02-09 15:30 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).