Re: [PATCH] tty: serial: amba-pl011: added RS485 support [v3] | linux-arm-kernel

quoted hunk

On 30. 12. 20, 4:16, Ivan Sistik wrote:
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -75,6 +75,17 @@ config SERIAL_AMBA_PL011_CONSOLE
  	  your boot loader (lilo or loadlin) about how to pass options to the
  	  kernel at boot time.)
  
+config SERIAL_AMBA_PL011_SOFT_RS485
+	bool "RS485 software direction switching for ARM AMBA PL011 serial"
+	depends on SERIAL_AMBA_PL011=y
+	help
+	  Enable RS485 software direction switching of driver enable (RTS pin)
+	  for ARM AMBA PL011 serial. AMBA PL011 does not have HW support for
+	  RS485. This driver use 2 hrtimers. One is used for rs485 delays.
"uses"

+	  Second one is used for polling of TX FIFO. There is not TX FIFO
"The second one"

"There is no"

+	  empty interrupt in PL011. Secondary timer is started by empty
"The secondary". But I am confused, 2 timers: one, second, and secondary 
  -- three timers?

+	  transmit buffer.
+
  config SERIAL_EARLYCON_ARM_SEMIHOST
  	bool "Early console using ARM semihosting"
  	depends on ARM64 || ARM

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 16720c97a..6a40e5bc5 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -46,6 +46,7 @@
  #include <linux/sizes.h>
  #include <linux/io.h>
  #include <linux/acpi.h>
+#include <linux/math64.h>
  
  #include "amba-pl011.h"
  
@@ -60,6 +61,18 @@
  #define UART_DR_ERROR		(UART011_DR_OE|UART011_DR_BE|UART011_DR_PE|UART011_DR_FE)
  #define UART_DUMMY_DR_RX	(1 << 16)
  
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+/*
+ * Enum with current status
+ */
+enum rs485_status {
+	rs485_receiving,
+	rs485_delay_before_send,
+	rs485_sending,
+	rs485_delay_after_send
These are too generic names.

+};
+#endif
You don't have to add ifdeffery to declarations.

+
  static u16 pl011_std_offsets[REG_ARRAY_SIZE] = {
  	[REG_DR] = UART01x_DR,
  	[REG_FR] = UART01x_FR,
@@ -270,6 +283,16 @@ struct uart_amba_port {
  	unsigned int		old_cr;		/* state during shutdown */
  	unsigned int		fixed_baud;	/* vendor-set fixed baud rate */
  	char			type[12];
+
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	enum rs485_status	rs485_current_status; /* status used for RTS */
+	enum rs485_status	rs485_next_status; /* this status after tick */
+	struct hrtimer		rs485_delay_timer;
+	struct hrtimer		rs485_tx_empty_poll_timer;
+	unsigned long		send_char_time;	/* send char (nanoseconds) */
+	bool			rs485_last_char_sending;
+#endif
+
  #ifdef CONFIG_DMA_ENGINE
  	/* DMA stuff */
  	bool			using_tx_dma;
@@ -306,6 +329,36 @@ static void pl011_write(unsigned int val, const struct uart_amba_port *uap,
  		writew_relaxed(val, addr);
  }
  
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+
+static void pl011_rs485_start_rts_delay(struct uart_amba_port *uap);
+
+static void rs485_set_rts_signal(struct uart_amba_port *uap, bool value)
+{
+	unsigned int rts_temp_cr;
+
+	rts_temp_cr = pl011_read(uap, REG_CR);
+
+	if (!value)
+		rts_temp_cr |= UART011_CR_RTS;
+	else
+		rts_temp_cr &= ~UART011_CR_RTS;
+
+	pl011_write(rts_temp_cr, uap, REG_CR);
+}
+
+void rs485_cancel_timers(struct uart_amba_port *uap)
Why not static? And too generic name. The same above.

+{
+	hrtimer_try_to_cancel(&(uap->rs485_delay_timer));
+	hrtimer_try_to_cancel(&(uap->rs485_tx_empty_poll_timer));
No need for the parentheses. They occur on many places in the patch. 
Have you run the patch through checkpatch?

+}
+
+bool rs485_tx_fifo_empty(struct uart_amba_port *uap)
+{
+	return (pl011_read(uap, REG_FR) & UART011_FR_TXFE);
detto.

+}
+#endif
+
  /*
   * Reads up to 256 characters from the FIFO or until it's empty and
   * inserts them into the TTY layer. Returns the number of characters
@@ -1301,6 +1354,11 @@ 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);
+
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	if (uap->port.rs485.flags & SER_RS485_ENABLED)
+		pl011_rs485_start_rts_delay(uap);
Could you do the check inside pl011_rs485_start_rts_delay and remove the 
ifdef completely?

+#endif
  }
  
  static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
@@ -1319,8 +1377,113 @@ static void pl011_start_tx(struct uart_port *port)
  	struct uart_amba_port *uap =
  	    container_of(port, struct uart_amba_port, port);
  
-	if (!pl011_dma_tx_start(uap))
-		pl011_start_tx_pio(uap);
+#define START_PL011_TX()				\
+	do {						\
+		if (!pl011_dma_tx_start(uap))		\
+			pl011_start_tx_pio(uap);	\
+	} while (0)
Defining a macro inside a function? No. This should be a separate 
function as well as the code below anyway.

+#ifndef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	START_PL011_TX();
+#else
+
+	if (uap->port.rs485.flags & SER_RS485_ENABLED) {
+		ktime_t ktime;
+
+		switch (uap->rs485_current_status) {
+		case rs485_delay_after_send:
+
+			rs485_cancel_timers(uap);
+
+			/* check if timer expired */
+			if (uap->rs485_current_status
+					!= rs485_delay_after_send) {
+				/* Timer expired and RTS is in wrong state.*/
+				uap->rs485_current_status
+					= rs485_delay_before_send;
+				uap->rs485_next_status = rs485_sending;
+
+				rs485_set_rts_signal(uap,
+					uap->port.rs485.flags
+						& SER_RS485_RTS_ON_SEND);
+
+				ktime = ktime_set(0,
+					  uap->port.rs485
+						.delay_rts_before_send
+					  * 1000000L);
+
+				hrtimer_start(
+					&(uap->rs485_delay_timer),
+					ktime,
+					HRTIMER_MODE_REL);
SPaghetti code. It will be eliminated by moving to a separate function 
suggested above.

+				return;
+			}
+
+			/* timer was stopped and driver can continue sending */
+			uap->rs485_current_status = rs485_sending;
+			uap->rs485_next_status = rs485_sending;
+
+			/* driver is already in sending state */
+			START_PL011_TX();
+			break;
+
+
+		case rs485_sending:
+			/* stop old timer. There can be running timer	*/
+			/* which is checking TX FIFO empty flag		*/
+			rs485_cancel_timers(uap);
+
+			/* driver is already in sending state */
+			START_PL011_TX();
+			break;
+
+		case rs485_receiving:
+		default:
+			/* stop old timer. There can be running timer	*/
+			/* which is checking TX FIFO empty flag		*/
+			rs485_cancel_timers(uap);
+
+			/* Set RTS */
+			rs485_set_rts_signal(uap,
+				     uap->port.rs485.flags
+					     & SER_RS485_RTS_ON_SEND);
+
+			if (uap->port.rs485.delay_rts_before_send == 0) {
+				/* Change state */
+				uap->rs485_current_status
+					= rs485_sending;
+				uap->rs485_next_status
+					= rs485_sending;
+
+				/* driver is in sending state */
+				START_PL011_TX();
+				break;
+			}
+
+			/* Change state */
+			uap->rs485_current_status
+				= rs485_delay_before_send;
+			uap->rs485_next_status = rs485_sending;
+
+			/* Start timer */
+			ktime = ktime_set(0,
+				  uap->port.rs485.delay_rts_before_send
+				  * 1000000L);
This is overcomplicated ms_to_ktime().

+			hrtimer_start(&(uap->rs485_delay_timer),
+				ktime,
+				HRTIMER_MODE_REL);
+			break;
+
+		case rs485_delay_before_send:
+			/* do nothing because delay timer should be running */
+			break;
+		}
+	} else {
+		START_PL011_TX();
+	}
+#endif
+
+#undef START_PL011_TX
  }
  
  static void pl011_stop_rx(struct uart_port *port)
@@ -1476,6 +1639,166 @@ static void check_apply_cts_event_workaround(struct uart_amba_port *uap)
  	dummy_read = pl011_read(uap, REG_ICR);
  }
  
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+
+/*
+ * Change state according to pending delay
+ * Locking: port is locked in this function
+ */
+static enum hrtimer_restart
+pl011_rs485_tx_poll_timer(struct hrtimer *timer)
+{
+	unsigned long flags;
+	ktime_t ktime;
+
+	struct uart_amba_port *uap =
+		container_of(timer, struct uart_amba_port,
+			     rs485_tx_empty_poll_timer);
+
+	spin_lock_irqsave(&uap->port.lock, flags);
+
+	if (!(uart_circ_empty(&uap->port.state->xmit))) {
+		spin_unlock_irqrestore(&uap->port.lock, flags);
+		return HRTIMER_NORESTART;
Create a label at the end and goto from here.

+	}
+
+	if (!rs485_tx_fifo_empty(uap) || !uap->rs485_last_char_sending) {
+		/*
+		 *  FIFO is empty but there is last char in transmit shift
+		 * register so we need one more tick
+		 */
+		uap->rs485_last_char_sending = rs485_tx_fifo_empty(uap);
+
+		hrtimer_forward_now(timer, ktime_set(0, uap->send_char_time));
ns_to_ktime()

+
+		spin_unlock_irqrestore(&uap->port.lock, flags);
+		return HRTIMER_RESTART;
+	}
+
+	/* Check if delay after send is set*/
+	if (uap->port.rs485.delay_rts_after_send == 0) {
+		/* Change state */
+		uap->rs485_current_status = rs485_receiving;
+		uap->rs485_next_status = rs485_receiving;
+
+		/* if there is no delay after send change RTS value*/
+		rs485_set_rts_signal(uap,
+			     uap->port.rs485.flags
+				     & SER_RS485_RTS_AFTER_SEND);
+
+		spin_unlock_irqrestore(&uap->port.lock, flags);
+		return HRTIMER_NORESTART;
+	}
+
+	/* Change state */
+	uap->rs485_current_status = rs485_delay_after_send;
+	uap->rs485_next_status = rs485_receiving;
+
+	/* RTS will be set in timer handler */
+
+	/* Start delay timer */
+	ktime = ktime_set(0, (uap->port.rs485.delay_rts_after_send
+			* 1000000L));
ms_to_ktime().

+	hrtimer_start(&(uap->rs485_delay_timer), ktime, HRTIMER_MODE_REL);
+
+	spin_unlock_irqrestore(&uap->port.lock, flags);
+	return HRTIMER_NORESTART;
+}
...
+static void pl011_rs485_start_rts_delay(struct uart_amba_port *uap)
+{
+	ktime_t ktime;
+
+	if (uap->rs485_current_status == rs485_receiving)
+		return;
+
+	/* if there is timeout in progress cancel it and start new */
+	hrtimer_try_to_cancel(&(uap->rs485_delay_timer));
+	hrtimer_try_to_cancel(&(uap->rs485_tx_empty_poll_timer));
+
+
+	if (!rs485_tx_fifo_empty(uap)
+			|| uap->port.rs485.delay_rts_after_send == 0) {
+		/*
+		 * Schedule validation timer if there is data in TX FIFO
+		 * because there is not TX FIFO empty interrupt
+		 */
+
+		uap->rs485_current_status = rs485_sending;
+		uap->rs485_next_status = rs485_sending;
+
+		uap->rs485_last_char_sending = false;
+
+		ktime = ktime_set(0, uap->send_char_time);
ns_to_ktime()

+		hrtimer_start(&(uap->rs485_tx_empty_poll_timer),
+			ktime,
+			HRTIMER_MODE_REL);
+		return;
+	}
+
+	/* Change state */
+	uap->rs485_current_status = rs485_delay_after_send;
+	uap->rs485_next_status = rs485_receiving;
+
+	/* RTS will be set in timer handler */
+
+	/* Start timer */
+	ktime = ktime_set(0, (uap->port.rs485.delay_rts_after_send
+			* 1000000L));
ms_to_ktime().

+
+	hrtimer_start(&(uap->rs485_delay_timer),
+		ktime,
+		HRTIMER_MODE_REL);
+}
+#endif
+
  static irqreturn_t pl011_int(int irq, void *dev_id)
  {
  	struct uart_amba_port *uap = dev_id;
@@ -1618,6 +1941,11 @@ static void pl011_quiesce_irqs(struct uart_port *port)
  	 */
  	pl011_write(pl011_read(uap, REG_IMSC) & ~UART011_TXIM, uap,
  		    REG_IMSC);
+
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	if (uap->port.rs485.flags & SER_RS485_ENABLED)
+		pl011_rs485_start_rts_delay(uap);
+#endif
The same as above.

  }
  
  static int pl011_get_poll_char(struct uart_port *port)
@@ -1690,6 +2018,27 @@ static int pl011_hwinit(struct uart_port *port)
  		if (plat->init)
  			plat->init();
  	}
+
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
Do this in a separate function and eliminate such ifdefs in the code.

+	/*
+	 * Initialize timers used for RS485
+	 */
+	hrtimer_init(&(uap->rs485_delay_timer),
+		CLOCK_MONOTONIC,
+		HRTIMER_MODE_REL);
+
+	uap->rs485_delay_timer.function = &pl011_rs485_timer;
+
+	hrtimer_init(&(uap->rs485_tx_empty_poll_timer),
+		CLOCK_MONOTONIC,
+		HRTIMER_MODE_REL);
+
+	uap->rs485_tx_empty_poll_timer.function = &pl011_rs485_tx_poll_timer;
+
+	uap->rs485_current_status = rs485_receiving;
+	rs485_set_rts_signal(uap, false);
+#endif
+
  	return 0;
  }
  
@@ -1873,6 +2222,16 @@ static void pl011_shutdown(struct uart_port *port)
  	struct uart_amba_port *uap =
  		container_of(port, struct uart_amba_port, port);
  
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	if (uap->port.rs485.flags & SER_RS485_ENABLED) {
+		hrtimer_try_to_cancel(&(uap->rs485_delay_timer));
+		hrtimer_try_to_cancel(&(uap->rs485_tx_empty_poll_timer));
+
+		uap->rs485_current_status = rs485_receiving;
+		rs485_set_rts_signal(uap, true);
+	}
+#endif
Separate function + no ifdef.

+
  	pl011_disable_interrupts(uap);
  
  	pl011_dma_shutdown(uap);
@@ -1955,6 +2314,24 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
  	unsigned long flags;
  	unsigned int baud, quot, clkdiv;
  
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	unsigned int transfer_bit_count;
+	unsigned long char_transfer_time;
+
+	/*
+	 * Calculate bit count which will be send
"sent"

+	 * by UART. It is used for calculation of
+	 * time required to start timer until TX FIFO (HW) is empty
Dot at the end.

+	 * There is not interrupt for FIFO empty in PL011.
"There is not an"
or
"There is no"

+	 * There is only FIFO empty flag in REG_FR.
+	 */
+	transfer_bit_count = 0;
+
+#define	ADD_DATA_BITS(bits)	(transfer_bit_count += bits)
This is ugly.
1) it's a macro.
2) it hides uses of transfer_bit_count from the reader.

+#else
+#define	ADD_DATA_BITS(bits)
+#endif
+
  	if (uap->vendor->oversampling)
  		clkdiv = 8;
  	else
@@ -1981,29 +2358,53 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
  	switch (termios->c_cflag & CSIZE) {
  	case CS5:
  		lcr_h = UART01x_LCRH_WLEN_5;
+		ADD_DATA_BITS(7);
  		break;
  	case CS6:
  		lcr_h = UART01x_LCRH_WLEN_6;
+		ADD_DATA_BITS(8);
  		break;
  	case CS7:
  		lcr_h = UART01x_LCRH_WLEN_7;
+		ADD_DATA_BITS(9);
  		break;
  	default: // CS8
  		lcr_h = UART01x_LCRH_WLEN_8;
+		ADD_DATA_BITS(10);
  		break;
  	}
-	if (termios->c_cflag & CSTOPB)
+
+	if (termios->c_cflag & CSTOPB) {
  		lcr_h |= UART01x_LCRH_STP2;
+		ADD_DATA_BITS(1);
+	}
+
  	if (termios->c_cflag & PARENB) {
  		lcr_h |= UART01x_LCRH_PEN;
+		ADD_DATA_BITS(1);
+
  		if (!(termios->c_cflag & PARODD))
  			lcr_h |= UART01x_LCRH_EPS;
+
  		if (termios->c_cflag & CMSPAR)
  			lcr_h |= UART011_LCRH_SPS;
  	}
+
+#undef ADD_DATA_BITS
+
  	if (uap->fifosize > 1)
  		lcr_h |= UART01x_LCRH_FEN;
  
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	/* Calculate time required to send one char (nanoseconds) */
+	char_transfer_time =
+		(unsigned long) div_u64(
+				mul_u32_u32(
+					(u32)transfer_bit_count,
+					(u32)NSEC_PER_SEC),
+				(u32)baud);
Wny do you cast all that?

+#endif
+
  	spin_lock_irqsave(&port->lock, flags);
  
  	/*
@@ -2020,6 +2421,11 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
  	old_cr = pl011_read(uap, REG_CR);
  	pl011_write(0, uap, REG_CR);
  
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	/* Update send_char_time in locked context */
+	uap->send_char_time = char_transfer_time;
+#endif
+
  	if (termios->c_cflag & CRTSCTS) {
  		if (old_cr & UART011_CR_RTS)
  			old_cr |= UART011_CR_RTSEN;
@@ -2122,6 +2528,47 @@ static void pl011_config_port(struct uart_port *port, int flags)
  	}
  }
  
+/*
+ * Configure RS485
+ * Locking: called with port lock held and IRQs disabled
+ */
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+static int pl011_config_rs485(struct uart_port *port,
+			      struct serial_rs485 *rs485)
+{
+	bool was_disabled;
+	struct uart_amba_port *uap =
+			container_of(port, struct uart_amba_port, port);
+
+	was_disabled = !(port->rs485.flags & SER_RS485_ENABLED);
Where is this used?

+	port->rs485.flags = rs485->flags;
+	port->rs485.delay_rts_after_send = rs485->delay_rts_after_send;
+	port->rs485.delay_rts_before_send = rs485->delay_rts_before_send;
+
+	if (port->rs485.flags & SER_RS485_ENABLED) {
+		unsigned int cr;
+
+		hrtimer_try_to_cancel(&(uap->rs485_delay_timer));
+		hrtimer_try_to_cancel(&(uap->rs485_tx_empty_poll_timer));
+
+		/* If RS485 is enabled, disable auto RTS */
+		cr = pl011_read(uap, REG_CR);
+		cr &= ~UART011_CR_RTSEN;
+		pl011_write(cr, uap, REG_CR);
+
+		uap->rs485_current_status = rs485_receiving;
+		rs485_set_rts_signal(uap,
+			     port->rs485.flags
+				     & SER_RS485_RTS_AFTER_SEND);
+	} else {
+		rs485_set_rts_signal(uap, true);
+	}
+
+	return 0;
+}
+#endif
+
  /*
   * verify the new serial_struct (for TIOCSSERIAL).
   */
@@ -2647,6 +3094,11 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
  	uap->port.irq = dev->irq[0];
  	uap->port.ops = &amba_pl011_pops;
  
+#ifdef CONFIG_SERIAL_AMBA_PL011_SOFT_RS485
+	uap->port.rs485_config = &pl011_config_rs485;
+	uap->port.rs485.flags = 0;	/* RS485 is not enabled by default */
+#endif
+
  	snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));
  
  	ret = pl011_setup_port(&dev->dev, uap, &dev->res, portnr);
@@ -2819,10 +3271,15 @@ static struct amba_driver pl011_driver = {
  
  static int __init pl011_init(void)
  {
+#if IS_ENABLED(CONFIG_SERIAL_AMBA_PL011_SOFT_RS485)
+	printk(KERN_INFO "Serial: AMBA PL011 UART driver with soft RS485 support\n");
+#else
  	printk(KERN_INFO "Serial: AMBA PL011 UART driver\n");
+#endif
Didn't Stefan already commented on this?

You can actually remove the print completely (in a separate patch).

  	if (platform_driver_register(&arm_sbsa_uart_platform_driver))
  		pr_warn("could not register SBSA UART platform driver\n");
+
And I saw a comment about added whitespace too.

  	return amba_driver_register(&pl011_driver);
  }
  
thanks,
-- 
js
suse labs

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help