[PATCH v2] serial: amba-pl011: add RS485 support

[PATCH v2] serial: amba-pl011: add RS485 support

[Lino Sanfilippo]

Add basic support for RS485: Provide a callback to configure RS485
settings. Handle the RS485 specific part in the functions
pl011_rs485_tx_start() and pl011_rs485_tx_stop() which extend the generic
start/stop callbacks.
Beside via IOCTL from userspace RS485 can be enabled by means of the
device tree property "rs485-enabled-at-boot-time".

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
This patch applies against Gregs tty-testing branch and was tested on a
Raspberry Pi CM3.

Changes in V2:
- clamp RTS delays to 100ms as suggested by Jiri Slaby
- instead of counting bits "by hand" use the new function tty_get_frame_size()
  (also suggested by Jiri)
- use the term RS485 consistently in the commit message
- remove one blank line


 drivers/tty/serial/amba-pl011.c | 152 +++++++++++++++++++++++++++++++-
 1 file changed, 150 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e14f3378b8a0..92c6b08d56fb 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -265,6 +265,8 @@ struct uart_amba_port {
 	unsigned int		old_cr;		/* state during shutdown */
 	unsigned int		fixed_baud;	/* vendor-set fixed baud rate */
 	char			type[12];
+	bool			rs485_tx_started;
+	unsigned int		rs485_tx_drain_interval; /* usecs */
 #ifdef CONFIG_DMA_ENGINE
 	/* DMA stuff */
 	bool			using_tx_dma;
@@ -275,6 +277,8 @@ struct uart_amba_port {
 #endif
 };
 
+static unsigned int pl011_tx_empty(struct uart_port *port);
+
 static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
 	unsigned int reg)
 {
@@ -1282,6 +1286,34 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
 #define pl011_dma_flush_buffer	NULL
 #endif
 
+static int pl011_rs485_tx_stop(struct uart_amba_port *uap)
+{
+	struct uart_port *port = &uap->port;
+	u32 cr;
+
+	/* Wait until hardware tx queue is empty */
+	while (!pl011_tx_empty(port))
+		udelay(uap->rs485_tx_drain_interval);
+
+	if (port->rs485.delay_rts_after_send)
+		mdelay(port->rs485.delay_rts_after_send);
+
+	cr = pl011_read(uap, REG_CR);
+
+	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+		cr &= ~UART011_CR_RTS;
+	else
+		cr |= UART011_CR_RTS;
+	/* Disable the transmitter and reenable the transceiver */
+	cr &= ~UART011_CR_TXE;
+	cr |= UART011_CR_RXE;
+	pl011_write(cr, uap, REG_CR);
+
+	uap->rs485_tx_started = false;
+
+	return 0;
+}
+
 static void pl011_stop_tx(struct uart_port *port)
 {
 	struct uart_amba_port *uap =
@@ -1290,6 +1322,9 @@ static void pl011_stop_tx(struct uart_port *port)
 	uap->im &= ~UART011_TXIM;
 	pl011_write(uap->im, uap, REG_IMSC);
 	pl011_dma_tx_stop(uap);
+
+	if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
+		pl011_rs485_tx_stop(uap);
 }
 
 static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
@@ -1380,6 +1415,31 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c,
 	return true;
 }
 
+static void pl011_rs485_tx_start(struct uart_amba_port *uap)
+{
+	struct uart_port *port = &uap->port;
+	u32 cr;
+
+	/* Enable transmitter */
+	cr = pl011_read(uap, REG_CR);
+	cr |= UART011_CR_TXE;
+	/* Disable receiver if half-duplex */
+	if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
+		cr &= ~UART011_CR_RXE;
+
+	if (port->rs485.flags & SER_RS485_RTS_ON_SEND)
+		cr &= ~UART011_CR_RTS;
+	else
+		cr |= UART011_CR_RTS;
+
+	pl011_write(cr, uap, REG_CR);
+
+	if (port->rs485.delay_rts_before_send)
+		mdelay(port->rs485.delay_rts_before_send);
+
+	uap->rs485_tx_started = true;
+}
+
 /* Returns true if tx interrupts have to be (kept) enabled  */
 static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
 {
@@ -1397,6 +1457,10 @@ static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
 		return false;
 	}
 
+	if ((uap->port.rs485.flags & SER_RS485_ENABLED) &&
+	    !uap->rs485_tx_started)
+		pl011_rs485_tx_start(uap);
+
 	/* If we are using DMA mode, try to send some characters. */
 	if (pl011_dma_tx_irq(uap))
 		return true;
@@ -1542,6 +1606,9 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	    container_of(port, struct uart_amba_port, port);
 	unsigned int cr;
 
+	if (port->rs485.flags & SER_RS485_ENABLED)
+		mctrl &= ~TIOCM_RTS;
+
 	cr = pl011_read(uap, REG_CR);
 
 #define	TIOCMBIT(tiocmbit, uartbit)		\
@@ -1763,7 +1830,17 @@ static int pl011_startup(struct uart_port *port)
 
 	/* restore RTS and DTR */
 	cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR);
-	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+	cr |= UART01x_CR_UARTEN | UART011_CR_RXE;
+
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+			cr &= ~UART011_CR_RTS;
+		else
+			cr |= UART011_CR_RTS;
+	} else {
+		cr |= UART011_CR_TXE;
+	}
+
 	pl011_write(cr, uap, REG_CR);
 
 	spin_unlock_irq(&uap->port.lock);
@@ -1864,6 +1941,9 @@ static void pl011_shutdown(struct uart_port *port)
 
 	pl011_dma_shutdown(uap);
 
+	if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
+		pl011_rs485_tx_stop(uap);
+
 	free_irq(uap->port.irq, uap);
 
 	pl011_disable_uart(uap);
@@ -1941,6 +2021,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	unsigned int lcr_h, old_cr;
 	unsigned long flags;
 	unsigned int baud, quot, clkdiv;
+	unsigned int bits;
 
 	if (uap->vendor->oversampling)
 		clkdiv = 8;
@@ -1991,18 +2072,29 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
 	if (uap->fifosize > 1)
 		lcr_h |= UART01x_LCRH_FEN;
 
+	bits = tty_get_frame_size(termios->c_cflag);
+
 	spin_lock_irqsave(&port->lock, flags);
 
 	/*
 	 * Update the per-port timeout.
 	 */
 	uart_update_timeout(port, termios->c_cflag, baud);
+	/*
+	 * Calculate the approximated time it takes to transmit one character
+	 * with the given baud rate. We use this as the poll interval when we
+	 * wait for the tx queue to empty.
+	 */
+	uap->rs485_tx_drain_interval = (bits * 1000 * 1000) / baud;
 
 	pl011_setup_status_masks(port, termios);
 
 	if (UART_ENABLE_MS(port, termios->c_cflag))
 		pl011_enable_ms(port);
 
+	if (port->rs485.flags & SER_RS485_ENABLED)
+		termios->c_cflag &= ~CRTSCTS;
+
 	/* first, disable everything */
 	old_cr = pl011_read(uap, REG_CR);
 	pl011_write(0, uap, REG_CR);
@@ -2124,6 +2216,40 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
 	return ret;
 }
 
+static int pl011_rs485_config(struct uart_port *port,
+			      struct serial_rs485 *rs485)
+{
+	struct uart_amba_port *uap =
+		container_of(port, struct uart_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) {
+		u32 cr = pl011_read(uap, REG_CR);
+
+		cr &= ~UART011_CR_RTSEN;
+		pl011_write(cr, uap, REG_CR);
+		port->status &= ~UPSTAT_AUTORTS;
+	}
+
+	return 0;
+}
+
 static const struct uart_ops amba_pl011_pops = {
 	.tx_empty	= pl011_tx_empty,
 	.set_mctrl	= pl011_set_mctrl,
@@ -2588,10 +2714,28 @@ static int pl011_find_free_port(void)
 	return -EBUSY;
 }
 
+static int pl011_get_rs485_mode(struct uart_amba_port *uap)
+{
+	struct uart_port *port = &uap->port;
+	struct serial_rs485 *rs485 = &port->rs485;
+	int ret;
+
+	ret = uart_get_rs485_mode(port);
+	if (ret)
+		return ret;
+
+	/* 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);
+
+	return 0;
+}
+
 static int pl011_setup_port(struct device *dev, struct uart_amba_port *uap,
 			    struct resource *mmiobase, int index)
 {
 	void __iomem *base;
+	int ret;
 
 	base = devm_ioremap_resource(dev, mmiobase);
 	if (IS_ERR(base))
@@ -2608,6 +2752,10 @@ static int pl011_setup_port(struct device *dev, struct uart_amba_port *uap,
 	uap->port.flags = UPF_BOOT_AUTOCONF;
 	uap->port.line = index;
 
+	ret = pl011_get_rs485_mode(uap);
+	if (ret)
+		return ret;
+
 	amba_ports[index] = uap;
 
 	return 0;
@@ -2665,7 +2813,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	uap->port.iotype = vendor->access_32b ? UPIO_MEM32 : UPIO_MEM;
 	uap->port.irq = dev->irq[0];
 	uap->port.ops = &amba_pl011_pops;
-
+	uap->port.rs485_config = pl011_rs485_config;
 	snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));
 
 	ret = pl011_setup_port(&dev->dev, uap, &dev->res, portnr);

base-commit: b61c8bf4694b5115766849378dcb8787ff54e65e
-- 
2.31.1

Re: [PATCH v2] serial: amba-pl011: add RS485 support

[Greg KH]

On Fri, Jun 18, 2021 at 04:51:53PM +0200, Lino Sanfilippo wrote:
> Add basic support for RS485: Provide a callback to configure RS485
> settings. Handle the RS485 specific part in the functions
> pl011_rs485_tx_start() and pl011_rs485_tx_stop() which extend the generic
> start/stop callbacks.
> Beside via IOCTL from userspace RS485 can be enabled by means of the
> device tree property "rs485-enabled-at-boot-time".
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
> This patch applies against Gregs tty-testing branch and was tested on a
> Raspberry Pi CM3.
> 
> Changes in V2:
> - clamp RTS delays to 100ms as suggested by Jiri Slaby
> - instead of counting bits "by hand" use the new function tty_get_frame_size()
>   (also suggested by Jiri)
> - use the term RS485 consistently in the commit message
> - remove one blank line
> 
> 
>  drivers/tty/serial/amba-pl011.c | 152 +++++++++++++++++++++++++++++++-
>  1 file changed, 150 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index e14f3378b8a0..92c6b08d56fb 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -265,6 +265,8 @@ struct uart_amba_port {
>  	unsigned int		old_cr;		/* state during shutdown */
>  	unsigned int		fixed_baud;	/* vendor-set fixed baud rate */
>  	char			type[12];
> +	bool			rs485_tx_started;
> +	unsigned int		rs485_tx_drain_interval; /* usecs */
>  #ifdef CONFIG_DMA_ENGINE
>  	/* DMA stuff */
>  	bool			using_tx_dma;
> @@ -275,6 +277,8 @@ struct uart_amba_port {
>  #endif
>  };
>  
> +static unsigned int pl011_tx_empty(struct uart_port *port);
> +
>  static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
>  	unsigned int reg)
>  {
> @@ -1282,6 +1286,34 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
>  #define pl011_dma_flush_buffer	NULL
>  #endif
>  
> +static int pl011_rs485_tx_stop(struct uart_amba_port *uap)
> +{
> +	struct uart_port *port = &uap->port;
> +	u32 cr;
> +
> +	/* Wait until hardware tx queue is empty */
> +	while (!pl011_tx_empty(port))
> +		udelay(uap->rs485_tx_drain_interval);

No way out if the hardware doesn't ever empty?  Shouldn't you have an
"upper bound" on this loop somehow?


> +
> +	if (port->rs485.delay_rts_after_send)
> +		mdelay(port->rs485.delay_rts_after_send);
> +
> +	cr = pl011_read(uap, REG_CR);
> +
> +	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +		cr &= ~UART011_CR_RTS;
> +	else
> +		cr |= UART011_CR_RTS;

Blank line here please.

> +	/* Disable the transmitter and reenable the transceiver */
> +	cr &= ~UART011_CR_TXE;
> +	cr |= UART011_CR_RXE;
> +	pl011_write(cr, uap, REG_CR);
> +
> +	uap->rs485_tx_started = false;
> +
> +	return 0;

Why does this function return a value if it can not fail and you do not
check the return value of it?

> +}
> +
>  static void pl011_stop_tx(struct uart_port *port)
>  {
>  	struct uart_amba_port *uap =
> @@ -1290,6 +1322,9 @@ static void pl011_stop_tx(struct uart_port *port)
>  	uap->im &= ~UART011_TXIM;
>  	pl011_write(uap->im, uap, REG_IMSC);
>  	pl011_dma_tx_stop(uap);
> +
> +	if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
> +		pl011_rs485_tx_stop(uap);

So, no check :(


>  }
>  
>  static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
> @@ -1380,6 +1415,31 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c,
>  	return true;
>  }
>  
> +static void pl011_rs485_tx_start(struct uart_amba_port *uap)
> +{
> +	struct uart_port *port = &uap->port;
> +	u32 cr;
> +
> +	/* Enable transmitter */
> +	cr = pl011_read(uap, REG_CR);
> +	cr |= UART011_CR_TXE;

Blank line please.

> +	/* Disable receiver if half-duplex */
> +	if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
> +		cr &= ~UART011_CR_RXE;
> +
> +	if (port->rs485.flags & SER_RS485_RTS_ON_SEND)
> +		cr &= ~UART011_CR_RTS;
> +	else
> +		cr |= UART011_CR_RTS;
> +
> +	pl011_write(cr, uap, REG_CR);
> +
> +	if (port->rs485.delay_rts_before_send)
> +		mdelay(port->rs485.delay_rts_before_send);
> +
> +	uap->rs485_tx_started = true;
> +}
> +
>  /* Returns true if tx interrupts have to be (kept) enabled  */
>  static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
>  {
> @@ -1397,6 +1457,10 @@ static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
>  		return false;
>  	}
>  
> +	if ((uap->port.rs485.flags & SER_RS485_ENABLED) &&
> +	    !uap->rs485_tx_started)
> +		pl011_rs485_tx_start(uap);
> +
>  	/* If we are using DMA mode, try to send some characters. */
>  	if (pl011_dma_tx_irq(uap))
>  		return true;
> @@ -1542,6 +1606,9 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  	    container_of(port, struct uart_amba_port, port);
>  	unsigned int cr;
>  
> +	if (port->rs485.flags & SER_RS485_ENABLED)
> +		mctrl &= ~TIOCM_RTS;
> +
>  	cr = pl011_read(uap, REG_CR);
>  
>  #define	TIOCMBIT(tiocmbit, uartbit)		\
> @@ -1763,7 +1830,17 @@ static int pl011_startup(struct uart_port *port)
>  
>  	/* restore RTS and DTR */
>  	cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR);
> -	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
> +	cr |= UART01x_CR_UARTEN | UART011_CR_RXE;
> +
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> +			cr &= ~UART011_CR_RTS;
> +		else
> +			cr |= UART011_CR_RTS;
> +	} else {
> +		cr |= UART011_CR_TXE;
> +	}
> +
>  	pl011_write(cr, uap, REG_CR);
>  
>  	spin_unlock_irq(&uap->port.lock);
> @@ -1864,6 +1941,9 @@ static void pl011_shutdown(struct uart_port *port)
>  
>  	pl011_dma_shutdown(uap);
>  
> +	if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
> +		pl011_rs485_tx_stop(uap);
> +
>  	free_irq(uap->port.irq, uap);
>  
>  	pl011_disable_uart(uap);
> @@ -1941,6 +2021,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
>  	unsigned int lcr_h, old_cr;
>  	unsigned long flags;
>  	unsigned int baud, quot, clkdiv;
> +	unsigned int bits;
>  
>  	if (uap->vendor->oversampling)
>  		clkdiv = 8;
> @@ -1991,18 +2072,29 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
>  	if (uap->fifosize > 1)
>  		lcr_h |= UART01x_LCRH_FEN;
>  
> +	bits = tty_get_frame_size(termios->c_cflag);
> +
>  	spin_lock_irqsave(&port->lock, flags);
>  
>  	/*
>  	 * Update the per-port timeout.
>  	 */
>  	uart_update_timeout(port, termios->c_cflag, baud);

Blank line

> +	/*
> +	 * Calculate the approximated time it takes to transmit one character
> +	 * with the given baud rate. We use this as the poll interval when we
> +	 * wait for the tx queue to empty.
> +	 */
> +	uap->rs485_tx_drain_interval = (bits * 1000 * 1000) / baud;
>  
>  	pl011_setup_status_masks(port, termios);
>  
>  	if (UART_ENABLE_MS(port, termios->c_cflag))
>  		pl011_enable_ms(port);
>  
> +	if (port->rs485.flags & SER_RS485_ENABLED)
> +		termios->c_cflag &= ~CRTSCTS;
> +
>  	/* first, disable everything */
>  	old_cr = pl011_read(uap, REG_CR);
>  	pl011_write(0, uap, REG_CR);
> @@ -2124,6 +2216,40 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
>  	return ret;
>  }
>  
> +static int pl011_rs485_config(struct uart_port *port,
> +			      struct serial_rs485 *rs485)
> +{
> +	struct uart_amba_port *uap =
> +		container_of(port, struct uart_amba_port, port);
> +
> +	/* pick sane settings if the user hasn't */
> +	if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==

Why the !! in an if statement?

> +	    !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {

Same here, why?

> +		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;

Blank line please.

> +	/* Make sure auto RTS is disabled */
> +	if (port->rs485.flags & SER_RS485_ENABLED) {
> +		u32 cr = pl011_read(uap, REG_CR);
> +
> +		cr &= ~UART011_CR_RTSEN;
> +		pl011_write(cr, uap, REG_CR);
> +		port->status &= ~UPSTAT_AUTORTS;
> +	}
> +
> +	return 0;
> +}
> +

thanks,

greg k-h

Re: [PATCH v2] serial: amba-pl011: add RS485 support

[Lino Sanfilippo]

Hi,

On 24.06.21 at 14:55, Greg KH wrote:
>>
>> +static int pl011_rs485_tx_stop(struct uart_amba_port *uap)
>> +{
>> +	struct uart_port *port = &uap->port;
>> +	u32 cr;
>> +
>> +	/* Wait until hardware tx queue is empty */
>> +	while (!pl011_tx_empty(port))
>> +		udelay(uap->rs485_tx_drain_interval);
>
> No way out if the hardware doesn't ever empty?  Shouldn't you have an
> "upper bound" on this loop somehow?

Yes, indeed. I will fix this.

>
>> +
>> +	if (port->rs485.delay_rts_after_send)
>> +		mdelay(port->rs485.delay_rts_after_send);
>> +
>> +	cr = pl011_read(uap, REG_CR);
>> +
>> +	if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>> +		cr &= ~UART011_CR_RTS;
>> +	else
>> +		cr |= UART011_CR_RTS;
>
> Blank line here please.

Ok.

>
>> +	/* Disable the transmitter and reenable the transceiver */
>> +	cr &= ~UART011_CR_TXE;
>> +	cr |= UART011_CR_RXE;
>> +	pl011_write(cr, uap, REG_CR);
>> +
>> +	uap->rs485_tx_started = false;
>> +
>> +	return 0;
>
> Why does this function return a value if it can not fail and you do not
> check the return value of it?

>> +}
>> +
>>  static void pl011_stop_tx(struct uart_port *port)
>>  {
>>  	struct uart_amba_port *uap =
>> @@ -1290,6 +1322,9 @@ static void pl011_stop_tx(struct uart_port *port)
>>  	uap->im &= ~UART011_TXIM;
>>  	pl011_write(uap->im, uap, REG_IMSC);
>>  	pl011_dma_tx_stop(uap);
>> +
>> +	if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
>> +		pl011_rs485_tx_stop(uap);
>
> So, no check :(
>

Ah, right. The return value is a leftover from an earlier version of the function. I will
correct this in the next patch version.

>
>>  }
>>
>>  static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
>> @@ -1380,6 +1415,31 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c,
>>  	return true;
>>  }
>>
>> +static void pl011_rs485_tx_start(struct uart_amba_port *uap)
>> +{
>> +	struct uart_port *port = &uap->port;
>> +	u32 cr;
>> +
>> +	/* Enable transmitter */
>> +	cr = pl011_read(uap, REG_CR);
>> +	cr |= UART011_CR_TXE;
>
> Blank line please.
>

Ok.

>> +
>>  	spin_lock_irqsave(&port->lock, flags);
>>
>>  	/*
>>  	 * Update the per-port timeout.
>>  	 */
>>  	uart_update_timeout(port, termios->c_cflag, baud);
>
> Blank line
>

Ok.

>>
>> +static int pl011_rs485_config(struct uart_port *port,
>> +			      struct serial_rs485 *rs485)
>> +{
>> +	struct uart_amba_port *uap =
>> +		container_of(port, struct uart_amba_port, port);
>> +
>> +	/* pick sane settings if the user hasn't */
>> +	if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>
> Why the !! in an if statement?
>
>> +	    !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>
> Same here, why?
>

This was copied from serial8250_em485_config(). But I think we can simply use

	if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
		rs485->flags &= ~SER_RS485_RTS_ON_SEND;
	else
		rs485->flags |= SER_RS485_RTS_ON_SEND;

instead. I will adjust the code accordingly.

>> +
>> +	if (port->rs485.flags & SER_RS485_ENABLED)
>> +		pl011_rs485_tx_stop(uap);
>> +
>> +	/* Set new configuration */
>> +	port->rs485 = *rs485;
>
> Blank line please.
>

Ok.

>
> thanks,
>
> greg k-h
>

Thank you for the review!

Regards,
Lino

Re: [PATCH v2] serial: amba-pl011: add RS485 support

[Jiri Slaby]

On 25. 06. 21, 2:15, Lino Sanfilippo wrote:
>>> +static int pl011_rs485_config(struct uart_port *port,
>>> +			      struct serial_rs485 *rs485)
>>> +{
>>> +	struct uart_amba_port *uap =
>>> +		container_of(port, struct uart_amba_port, port);
>>> +
>>> +	/* pick sane settings if the user hasn't */
>>> +	if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>>
>> Why the !! in an if statement?
>>
>>> +	    !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>>
>> Same here, why?
>>
> 
> This was copied from serial8250_em485_config(). But I think we can simply use
> 
> 	if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
> 		rs485->flags &= ~SER_RS485_RTS_ON_SEND;
> 	else
> 		rs485->flags |= SER_RS485_RTS_ON_SEND;
> 
> instead. I will adjust the code accordingly.

This is different. You want to set ON_SEND when none is set. And unset 
AFTER_SEND when both are set. In your code, when both are set, you leave 
AFTER_SEND.

regards,
-- 
js
suse labs

Re: [PATCH v2] serial: amba-pl011: add RS485 support

[Lino Sanfilippo]

Hi,

> On 25. 06. 21, 2:15, Lino Sanfilippo wrote:
> >>> +static int pl011_rs485_config(struct uart_port *port,
> >>> +			      struct serial_rs485 *rs485)
> >>> +{
> >>> +	struct uart_amba_port *uap =
> >>> +		container_of(port, struct uart_amba_port, port);
> >>> +
> >>> +	/* pick sane settings if the user hasn't */
> >>> +	if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> >>
> >> Why the !! in an if statement?
> >>
> >>> +	    !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
> >>
> >> Same here, why?
> >>
> >
> > This was copied from serial8250_em485_config(). But I think we can simply use
> >
> > 	if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
> > 		rs485->flags &= ~SER_RS485_RTS_ON_SEND;
> > 	else
> > 		rs485->flags |= SER_RS485_RTS_ON_SEND;
> >
> > instead. I will adjust the code accordingly.
>
> This is different. You want to set ON_SEND when none is set. And unset
> AFTER_SEND when both are set. In your code, when both are set, you leave
> AFTER_SEND.
>
> regards,
> --
> js
> suse labs
>

Thats right, the logic has slightly changed. I thought this does not matter as long as
we make sure that exactly one of both ON_SEND or AFTER_SEND is set. We can stick with the logic
in serial8250_em485_config() (i.e. always set ON_SEND and delete AFTER_SEND in case
of an invalid setting), but I think this will require more than the four lines that we
have now (especially if we want to avoid the !!).

Thanks,
Lino

Re: [PATCH v2] serial: amba-pl011: add RS485 support

[Jiri Slaby]

Hi,

On 25. 06. 21, 13:21, Lino Sanfilippo wrote:
> Thats right, the logic has slightly changed. I thought this does not matter as long as
> we make sure that exactly one of both ON_SEND or AFTER_SEND is set.

I actually don't know. But I would remain consistent with other drivers.

> We can stick with the logic
> in serial8250_em485_config() (i.e. always set ON_SEND and delete AFTER_SEND in case
> of an invalid setting), but I think this will require more than the four lines that we
> have now (especially if we want to avoid the !!).

What if you used only a single !?

thanks,
-- 
js
suse labs

Re: [PATCH v2] serial: amba-pl011: add RS485 support

[Lino Sanfilippo]

Hi,

On 26.06.21 at 05:41, Jiri Slaby wrote:

> On 25. 06. 21, 13:21, Lino Sanfilippo wrote:
>> Thats right, the logic has slightly changed. I thought this does not matter as long as
>> we make sure that exactly one of both ON_SEND or AFTER_SEND is set.
>
> I actually don't know. But I would remain consistent with other drivers.
>
>> We can stick with the logic
>> in serial8250_em485_config() (i.e. always set ON_SEND and delete AFTER_SEND in case
>> of an invalid setting), but I think this will require more than the four lines that we
>> have now (especially if we want to avoid the !!).
>
> What if you used only a single !?
>

You are right, this is the easiest way to keep the current logic. I will do that.

Thanks,
Lino