This patch series is an attempt to simplify rs485 implementation in drivers
by moving the following tasks out of the drivers into the serial core:
- ensure sane RTS settings: in case of an invalid configuration (both RTS
after send and RTS on send set or both unset) enable RTS on send and
disable RTS after send
- nullify the padding field of the serial_rs485 struct before it is
returned to userspace
- copy the configuration stored in the serial_rs485 struct to the port
configuration if setting the configuration in the driver was successfull
- limit the RTS delays to 100ms
Redundant code has been removed from the following drivers for now:
- atmel
- fsl_lpuart
- amba
- imx
- max310x
- omap-serial
- sc16is7xx
- stm32-usart
The code has been tested with the amba pl011 driver.
Changes in v2:
- use a makro for max RTS delays and comment it (as requested by Jiri)
- add a comment concerning the memset of a structures padding field
- correct typos in the commit message (found by Uwe)
- rephrase all commit messages to make more clear that function
uart_set_rs485_config() has been extended by checks and other
functionalities (as requested by Uwe)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
In uart_set_rs485_config() the serial core already
- ensures that only one of both options RTS on send or RTS after send is
set
- nullifies the padding field of the passed serial_rs485 struct
- clamps the RTS delays
- assigns the passed serial_rs485 struct to the uart port
So remove these tasks from the code of the drivers rs485_config() function
to avoid redundancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/amba-pl011.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
@@ -2170,25 +2170,11 @@ static int pl011_rs485_config(struct uart_port *port,structuart_amba_port*uap=container_of(port,structuart_amba_port,port);-/* pick sane settings if the user hasn't */-if(!(rs485->flags&SER_RS485_RTS_ON_SEND)==-!(rs485->flags&SER_RS485_RTS_AFTER_SEND)){-rs485->flags|=SER_RS485_RTS_ON_SEND;-rs485->flags&=~SER_RS485_RTS_AFTER_SEND;-}-/* clamp the delays to [0, 100ms] */-rs485->delay_rts_before_send=min(rs485->delay_rts_before_send,100U);-rs485->delay_rts_after_send=min(rs485->delay_rts_after_send,100U);-memset(rs485->padding,0,sizeof(rs485->padding));-if(port->rs485.flags&SER_RS485_ENABLED)pl011_rs485_tx_stop(uap);-/* Set new configuration */-port->rs485=*rs485;-/* Make sure auto RTS is disabled */-if(port->rs485.flags&SER_RS485_ENABLED){+if(rs485->flags&SER_RS485_ENABLED){u32cr=pl011_read(uap,REG_CR);cr&=~UART011_CR_RTSEN;
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
In uart_set_rs485_config() the serial core already nullifies the padding
field of the passed serial_rs485 struct before returning it to userspace.
Doing the same in the drivers rs485_config() function is redundant, so
remove the concerning memset in this function.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/max310x.c | 1 -
1 file changed, 1 deletion(-)
In uart_set_rs485_config() the serial core already clamps the RTS delays.
It also assigns the passed serial_rs485 struct to the uart port.
So remove these tasks from the drivers rs485_config() function to avoid
redundancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/omap-serial.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
@@ -1350,18 +1350,11 @@ serial_omap_config_rs485(struct uart_port *port, struct serial_rs485 *rs485)up->ier=0;serial_out(up,UART_IER,0);-/* Clamp the delays to [0, 100ms] */-rs485->delay_rts_before_send=min(rs485->delay_rts_before_send,100U);-rs485->delay_rts_after_send=min(rs485->delay_rts_after_send,100U);--/* store new config */-port->rs485=*rs485;-if(up->rts_gpiod){/* enable / disable rts */-val=(port->rs485.flags&SER_RS485_ENABLED)?+val=(rs485->flags&SER_RS485_ENABLED)?SER_RS485_RTS_AFTER_SEND:SER_RS485_RTS_ON_SEND;-val=(port->rs485.flags&val)?1:0;+val=(rs485->flags&val)?1:0;gpiod_set_value(up->rts_gpiod,val);}
@@ -1372,7 +1365,7 @@ serial_omap_config_rs485(struct uart_port *port, struct serial_rs485 *rs485)/* If RS-485 is disabled, make sure the THR interrupt is fired when*TXFIFOisbelowthetriggerlevel.*/-if(!(port->rs485.flags&SER_RS485_ENABLED)&&+if(!(rs485->flags&SER_RS485_ENABLED)&&(up->scr&OMAP_UART_SCR_TX_EMPTY)){up->scr&=~OMAP_UART_SCR_TX_EMPTY;serial_out(up,UART_OMAP_SCR,up->scr);
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
In uart_set_rs485_config() the serial core already ensures that only one of
both options RTS on send or RTS after send is set. It also assigns the
passed serial_rs485 struct to the uart port.
So remove the check and the assignment from the drivers rs485_config()
function to avoid redundancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/stm32-usart.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
Several drivers that support setting the RS485 configuration via userspace
implement one or more of the following tasks:
- in case of an invalid RTS configuration (both RTS after send and RTS on
send set or both unset) fall back to enable RTS on send and disable RTS
after send
- nullify the padding field of the returned serial_rs485 struct
- copy the configuration into the uart port struct
- limit RTS delays to 100 ms
Move these tasks into the serial core to make them generic and to provide
a consistent behaviour among all drivers.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
include/uapi/linux/serial.h | 3 +++
2 files changed, 21 insertions(+)
@@ -1282,8 +1282,26 @@ static int uart_set_rs485_config(struct uart_port *port,if(copy_from_user(&rs485,rs485_user,sizeof(*rs485_user)))return-EFAULT;+/* pick sane settings if the user hasn't */+if(!(rs485.flags&SER_RS485_RTS_ON_SEND)==+!(rs485.flags&SER_RS485_RTS_AFTER_SEND)){+rs485.flags|=SER_RS485_RTS_ON_SEND;+rs485.flags&=~SER_RS485_RTS_AFTER_SEND;+}++rs485.delay_rts_before_send=min_t(unsignedint,+rs485.delay_rts_before_send,+SER_RS485_MAX_RTS_DELAY);+rs485.delay_rts_after_send=min_t(unsignedint,+rs485.delay_rts_after_send,+SER_RS485_MAX_RTS_DELAY);+/* Return clean padding area to userspace */+memset(rs485.padding,0,sizeof(rs485.padding));+spin_lock_irqsave(&port->lock,flags);ret=port->rs485_config(port,&rs485);+if(!ret)+port->rs485=rs485;spin_unlock_irqrestore(&port->lock,flags);if(ret)returnret;
@@ -128,6 +128,9 @@ struct serial_rs485 {(ifsupported)*/__u32delay_rts_before_send;/* Delay before send (milliseconds) */__u32delay_rts_after_send;/* Delay after send (milliseconds) */+#define SER_RS485_MAX_RTS_DELAY 100 /* Max time with active+RTSbefore/after+datasent(msecs)*/__u32padding[5];/* Memory is cheap, new structsarearoyalPITA..*/};
In uart_set_rs485_config() the serial core already ensures that only one of
both options RTS on send or RTS after send is set.
So remove this check from the drivers rs485_config() function to avoid
redundancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/sc16is7xx.c | 10 ----------
1 file changed, 10 deletions(-)
@@ -959,16 +959,6 @@ static int sc16is7xx_config_rs485(struct uart_port *port,structsc16is7xx_one*one=to_sc16is7xx_one(port,port);if(rs485->flags&SER_RS485_ENABLED){-boolrts_during_rx,rts_during_tx;--rts_during_rx=rs485->flags&SER_RS485_RTS_AFTER_SEND;-rts_during_tx=rs485->flags&SER_RS485_RTS_ON_SEND;--if(rts_during_rx==rts_during_tx)-dev_err(port->dev,-"unsupported RTS signalling on_send:%d after_send:%d - exactly one of RS485 RTS flags should be set\n",-rts_during_tx,rts_during_rx);-/**RTSsignalishandledbyHW,it'stimingcan'tbeinfluenced.*However,it'ssometimesusefultodelayTXevenwithoutRTS
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
In uart_set_rs485_config() the serial core already assigns the passed
serial_rs485 struct to the uart port.
So remove the assignment in the drivers rs485_config() function to avoid
reduncancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/imx.c | 2 --
1 file changed, 2 deletions(-)
In uart_set_rs485_config() the serial core already ensures that only one of
both options RTS on send or RTS after send is set. It also assigns the
passed serial_rs485 struct to the uart port.
So remove the check and the assignment from the drivers rs485_config()
function to avoid redundancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/fsl_lpuart.c | 32 --------------------------------
1 file changed, 32 deletions(-)
@@ -1377,19 +1377,6 @@ static int lpuart_config_rs485(struct uart_port *port,/* Enable auto RS-485 RTS mode */modem|=UARTMODEM_TXRTSE;-/*-*RTSneedstobelogicHIGHeitherduringtransfer_or_after-*transfer,othervariantsarenotsupportedbythehardware.-*/--if(!(rs485->flags&(SER_RS485_RTS_ON_SEND|-SER_RS485_RTS_AFTER_SEND)))-rs485->flags|=SER_RS485_RTS_ON_SEND;--if(rs485->flags&SER_RS485_RTS_ON_SEND&&-rs485->flags&SER_RS485_RTS_AFTER_SEND)-rs485->flags&=~SER_RS485_RTS_AFTER_SEND;-/**ThehardwaredefaultstoRTSlogicHIGHwhiletransfer.*SwitchpolarityincaseRTSshallbelogicHIGH
@@ -1402,9 +1389,6 @@ static int lpuart_config_rs485(struct uart_port *port,modem|=UARTMODEM_TXRTSPOL;}-/* Store the new configuration */-sport->port.rs485=*rs485;-writeb(modem,sport->port.membase+UARTMODEM);return0;}
@@ -1428,19 +1412,6 @@ static int lpuart32_config_rs485(struct uart_port *port,/* Enable auto RS-485 RTS mode */modem|=UARTMODEM_TXRTSE;-/*-*RTSneedstobelogicHIGHeitherduringtransfer_or_after-*transfer,othervariantsarenotsupportedbythehardware.-*/--if(!(rs485->flags&(SER_RS485_RTS_ON_SEND|-SER_RS485_RTS_AFTER_SEND)))-rs485->flags|=SER_RS485_RTS_ON_SEND;--if(rs485->flags&SER_RS485_RTS_ON_SEND&&-rs485->flags&SER_RS485_RTS_AFTER_SEND)-rs485->flags&=~SER_RS485_RTS_AFTER_SEND;-/**ThehardwaredefaultstoRTSlogicHIGHwhiletransfer.*SwitchpolarityincaseRTSshallbelogicHIGH
@@ -1453,9 +1424,6 @@ static int lpuart32_config_rs485(struct uart_port *port,modem|=UARTMODEM_TXRTSPOL;}-/* Store the new configuration */-sport->port.rs485=*rs485;-lpuart32_write(&sport->port,modem,UARTMODIR);return0;}
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
In uart_set_rs485_config() the serial core already assigns the passed
serial_rs485 struct to the uart port.
So remove the assignment from the drivers rs485_config() function to avoid
redundancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/atmel_serial.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
@@ -299,11 +299,9 @@ static int atmel_config_rs485(struct uart_port *port,/* Resetting serial mode to RS232 (0x0) */mode&=~ATMEL_US_USMODE;-port->rs485=*rs485conf;-if(rs485conf->flags&SER_RS485_ENABLED){dev_dbg(port->dev,"Setting UART to RS485\n");-if(port->rs485.flags&SER_RS485_RX_DURING_TX)+if(rs485conf->flags&SER_RS485_RX_DURING_TX)atmel_port->tx_done_mask=ATMEL_US_TXRDY;elseatmel_port->tx_done_mask=ATMEL_US_TXEMPTY;
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Feb 16, 2022 at 01:18:01AM +0100, Lino Sanfilippo wrote:
In uart_set_rs485_config() the serial core already assigns the passed
serial_rs485 struct to the uart port.
So remove the assignment in the drivers rs485_config() function to avoid
reduncancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
From: Richard Genoud <hidden> Date: 2022-02-17 09:22:13
Le 16/02/2022 à 01:18, Lino Sanfilippo a écrit :
In uart_set_rs485_config() the serial core already assigns the passed
serial_rs485 struct to the uart port.
So remove the assignment from the drivers rs485_config() function to avoid
redundancy.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
@@ -299,11 +299,9 @@ static int atmel_config_rs485(struct uart_port *port,/* Resetting serial mode to RS232 (0x0) */mode&=~ATMEL_US_USMODE;-port->rs485=*rs485conf;-if(rs485conf->flags&SER_RS485_ENABLED){dev_dbg(port->dev,"Setting UART to RS485\n");-if(port->rs485.flags&SER_RS485_RX_DURING_TX)+if(rs485conf->flags&SER_RS485_RX_DURING_TX)atmel_port->tx_done_mask=ATMEL_US_TXRDY;elseatmel_port->tx_done_mask=ATMEL_US_TXEMPTY;
On Wed, Feb 16, 2022 at 01:17:55AM +0100, Lino Sanfilippo wrote:
quoted hunk
Several drivers that support setting the RS485 configuration via userspace
implement one or more of the following tasks:
- in case of an invalid RTS configuration (both RTS after send and RTS on
send set or both unset) fall back to enable RTS on send and disable RTS
after send
- nullify the padding field of the returned serial_rs485 struct
- copy the configuration into the uart port struct
- limit RTS delays to 100 ms
Move these tasks into the serial core to make them generic and to provide
a consistent behaviour among all drivers.
Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
drivers/tty/serial/serial_core.c | 18 ++++++++++++++++++
include/uapi/linux/serial.h | 3 +++
2 files changed, 21 insertions(+)
@@ -1282,8 +1282,26 @@ static int uart_set_rs485_config(struct uart_port *port,if(copy_from_user(&rs485,rs485_user,sizeof(*rs485_user)))return-EFAULT;+/* pick sane settings if the user hasn't */+if(!(rs485.flags&SER_RS485_RTS_ON_SEND)==+!(rs485.flags&SER_RS485_RTS_AFTER_SEND)){+rs485.flags|=SER_RS485_RTS_ON_SEND;+rs485.flags&=~SER_RS485_RTS_AFTER_SEND;+}++rs485.delay_rts_before_send=min_t(unsignedint,+rs485.delay_rts_before_send,+SER_RS485_MAX_RTS_DELAY);+rs485.delay_rts_after_send=min_t(unsignedint,+rs485.delay_rts_after_send,+SER_RS485_MAX_RTS_DELAY);+/* Return clean padding area to userspace */+memset(rs485.padding,0,sizeof(rs485.padding));+spin_lock_irqsave(&port->lock,flags);ret=port->rs485_config(port,&rs485);+if(!ret)+port->rs485=rs485;spin_unlock_irqrestore(&port->lock,flags);if(ret)returnret;
@@ -128,6 +128,9 @@ struct serial_rs485 {(ifsupported)*/__u32delay_rts_before_send;/* Delay before send (milliseconds) */__u32delay_rts_after_send;/* Delay after send (milliseconds) */+#define SER_RS485_MAX_RTS_DELAY 100 /* Max time with active+RTSbefore/after+datasent(msecs)*/
Why is this a userspace value now? What can userspace do with this
number? Once we add this, it's fixed for forever.
thanks,
greg k-h
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -128,6 +128,9 @@ struct serial_rs485 {(ifsupported)*/__u32delay_rts_before_send;/* Delay before send (milliseconds) */__u32delay_rts_after_send;/* Delay after send (milliseconds) */+#define SER_RS485_MAX_RTS_DELAY 100 /* Max time with active+RTSbefore/after+datasent(msecs)*/
Why is this a userspace value now? What can userspace do with this
number? Once we add this, it's fixed for forever.
thanks,
greg k-h
Ok, I think we do not really need to expose it to userspace, since we
clamp the delay anyway if it is too big. I will correct this in the next
patch version.
Regards,
Lino
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel